[PATCH] usb: gadget: function: use after free in printer_close()

[PATCH] usb: gadget: function: use after free in printer_close()

[Dan Carpenter]

The printer_dev_free() function frees "dev" but then it is dereferenced
by the debug code on the next line.  The debug printk only prints the
function name so it's probably okay to just delete it.

Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/usb/gadget/function/f_printer.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index a881c69b1f2b..7354bfe1e682 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -382,7 +382,6 @@ printer_close(struct inode *inode, struct file *fd)
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	kref_put(&dev->kref, printer_dev_free);
-	DBG(dev, "printer_close\n");
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH] usb: gadget: function: use after free in printer_close()

[Andrzej Pietrasiewicz]

Hi Dan,

W dniu 18.11.2022 o 12:47, Dan Carpenter pisze:
> The printer_dev_free() function frees "dev" but then it is dereferenced
> by the debug code on the next line.  The debug printk only prints the
> function name so it's probably okay to just delete it.
> 
> Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/usb/gadget/function/f_printer.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> index a881c69b1f2b..7354bfe1e682 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -382,7 +382,6 @@ printer_close(struct inode *inode, struct file *fd)
>   	spin_unlock_irqrestore(&dev->lock, flags);
>   
>   	kref_put(&dev->kref, printer_dev_free);
> -	DBG(dev, "printer_close\n");

I think that if you delete the DBG() here, it should also be deleted in
printer_open(). Alternatively this patch should reverse the order of
calls to kref_put() and DBG().

Regards,

Andrzej

>   
>   	return 0;
>   }

[PATCH] usb: gadget: function: use after free in printer_close()

[Dan Carpenter]

The printer_dev_free() function frees "dev" but then it is dereferenced
by the debug code on the next line.  Flip the order to avoid the use after
free.

Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: In the v1, I just deleted the printk but Andrzej thought it was
worth preserving.

 drivers/usb/gadget/function/f_printer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index a881c69b1f2b..01e842e1ba2f 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -381,8 +381,8 @@ printer_close(struct inode *inode, struct file *fd)
 	dev->printer_status &= ~PRINTER_SELECTED;
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	kref_put(&dev->kref, printer_dev_free);
 	DBG(dev, "printer_close\n");
+	kref_put(&dev->kref, printer_dev_free);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH] usb: gadget: function: use after free in printer_close()

[Andrzej Pietrasiewicz]

Hi Dan,

I'm fine with either symmetrically removing the DBG() from "printer_open()"
or with this version of the patch.

It seems to me that this version better fits "fixing UAF", though.
Whether the driver is too verbose is another matter, and if it is,
it deserves its own patch because DBG() invocations are sprinkled
here and there.

W dniu 21.11.2022 o 15:44, Dan Carpenter pisze:
> The printer_dev_free() function frees "dev" but then it is dereferenced
> by the debug code on the next line.  Flip the order to avoid the use after
> free.
> 
> Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>


> ---
> v2: In the v1, I just deleted the printk but Andrzej thought it was
> worth preserving.
> 
>   drivers/usb/gadget/function/f_printer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> index a881c69b1f2b..01e842e1ba2f 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -381,8 +381,8 @@ printer_close(struct inode *inode, struct file *fd)
>   	dev->printer_status &= ~PRINTER_SELECTED;
>   	spin_unlock_irqrestore(&dev->lock, flags);
>   
> -	kref_put(&dev->kref, printer_dev_free);
>   	DBG(dev, "printer_close\n");
> +	kref_put(&dev->kref, printer_dev_free);
>   
>   	return 0;
>   }

Re: [PATCH] usb: gadget: function: use after free in printer_close()

[Dan Carpenter]

On Mon, Nov 21, 2022 at 04:32:52PM +0100, Andrzej Pietrasiewicz wrote:
> Hi Dan,
> 
> I'm fine with either symmetrically removing the DBG() from "printer_open()"
> or with this version of the patch.
> 
> It seems to me that this version better fits "fixing UAF", though.
> Whether the driver is too verbose is another matter, and if it is,
> it deserves its own patch because DBG() invocations are sprinkled
> here and there.

It is too verbose, but I'm trying to cut my kernel work to an hour a day
and then all day Friday so I don't have time to clean to do clean up
work.  A UAF is sort of high value but clean up is endless.

I obviously considered this as v1 but thought deleting was better.  I
still do.  :)  But it's not worth spending time on.

> 
> W dniu 21.11.2022 o 15:44, Dan Carpenter pisze:
> > The printer_dev_free() function frees "dev" but then it is dereferenced
> > by the debug code on the next line.  Flip the order to avoid the use after
> > free.
> > 
> > Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> Acked-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Thanks!

regards,
dan carpenter

Re: [PATCH] usb: gadget: function: use after free in printer_close()

[Andrzej Pietrasiewicz]

Hi Dan,

W dniu 22.11.2022 o 08:00, Dan Carpenter pisze:
> On Mon, Nov 21, 2022 at 04:32:52PM +0100, Andrzej Pietrasiewicz wrote:
>> Hi Dan,
>>
>> I'm fine with either symmetrically removing the DBG() from "printer_open()"
>> or with this version of the patch.
>>
>> It seems to me that this version better fits "fixing UAF", though.
>> Whether the driver is too verbose is another matter, and if it is,
>> it deserves its own patch because DBG() invocations are sprinkled
>> here and there.
> 
> It is too verbose, but I'm trying to cut my kernel work to an hour a day
> and then all day Friday so I don't have time to clean to do clean up
> work.  A UAF is sort of high value but clean up is endless.
> 

I volunteer to reduce the amount of debug messages it produces.

Andrzej

> I obviously considered this as v1 but thought deleting was better.  I
> still do.  :)  But it's not worth spending time on.
> 
>>
>> W dniu 21.11.2022 o 15:44, Dan Carpenter pisze:
>>> The printer_dev_free() function frees "dev" but then it is dereferenced
>>> by the debug code on the next line.  Flip the order to avoid the use after
>>> free.
>>>
>>> Fixes: e8d5f92b8d30 ("usb: gadget: function: printer: fix use-after-free in __lock_acquire")
>>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>
>> Acked-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> 
> Thanks!
> 
> regards,
> dan carpenter
>