Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 20 Nov 2023 10:06:10 +0100 | Subject | Re: [PATCH] nfc: virtual_ncidev: Add variable to check if ndev is running | From | Krzysztof Kozlowski <> |
| |
On 20/11/2023 05:47, Bongsu Jeon wrote: > > On 20/11/2023 01:47, Nguyen Dinh Phi wrote: > >> syzbot reported an memory leak that happens when an skb is add to >> send_buff after virtual nci closed. >> This patch adds a variable to track if the ndev is running before >> handling new skb in send function. >> >> Reported-by: syzbot+6eb09d75211863f15e3e@syzkaller.appspotmail.com >> Closes: https://lore.kernel.org/lkml/00000000000075472b06007df4fb@google.com >> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com> >> --- >> drivers/nfc/virtual_ncidev.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c >> index b027be0b0b6f..ac8226db54e2 100644 >> --- a/drivers/nfc/virtual_ncidev.c >> +++ b/drivers/nfc/virtual_ncidev.c >> @@ -20,26 +20,31 @@ >> NFC_PROTO_ISO14443_MASK | \ >> NFC_PROTO_ISO14443_B_MASK | \ >> NFC_PROTO_ISO15693_MASK) >> +#define NCIDEV_RUNNING 0 > This define isn't used. > >> >> struct virtual_nci_dev { >> struct nci_dev *ndev; >> struct mutex mtx; >> struct sk_buff *send_buff; >> struct wait_queue_head wq; >> + bool running; >> }; >> >> static int virtual_nci_open(struct nci_dev *ndev) >> { >> + struct virtual_nci_dev *vdev = nci_get_drvdata(ndev); >> + >> + vdev->running = true; >> return 0; >> } >> >> static int virtual_nci_close(struct nci_dev *ndev) >> { >> struct virtual_nci_dev *vdev = nci_get_drvdata(ndev); >> - >> mutex_lock(&vdev->mtx); >> kfree_skb(vdev->send_buff); >> vdev->send_buff = NULL; >> + vdev->running = false; >> mutex_unlock(&vdev->mtx); >> >> return 0; >> @@ -50,7 +55,7 @@ static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb) >> struct virtual_nci_dev *vdev = nci_get_drvdata(ndev); >> >> mutex_lock(&vdev->mtx); >> - if (vdev->send_buff) { >> + if (vdev->send_buff || !vdev->running) { > > Dear Krzysztof, > > I agree this defensive code. > But i think NFC submodule has to avoid this situation.(calling send function of closed nci_dev) > Could you check this?
This code looks not effective. At this point vdev->send_buff is always false, so the additional check would not bring any value.
I don't see this fixing anything. Syzbot also does not seem to agree.
Nguyen, please test your patches against syzbot *before* sending them. If you claim this fixes the report, please provide me the link to syzbot test results confirming it is fixed.
I looked at syzbot dashboard and do not see this issue fixed with this patch.
Best regards, Krzysztof
| ![\](/images/icornerr.gif) |