[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.  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
> 

[PATCH] usb: gadget: function: Simplify error messaging in printer open/close

[Andrzej Pietrasiewicz]

Don't issue any messages if printer_open() is successful.
Also don't issue them if unsuccessful - the error code is propagated
to the calling layers and should be acted on appropriately there. Just as
it is with the -ENODEV case.

For those who really want this message leave an option to compile-in
with composite framework's VDBG() by uncommenting #define VERBOSE_DEBUG.

While at it, visually detach the "return ret;" statement.

Use __func__ instead of explicitly hardcoding the function name. This, in
turn makes checkpatch issue this for the message in printer_close():

WARNING: Unnecessary ftrace-like logging - prefer using ftrace
54: FILE: drivers/usb/gadget/function/f_printer.c:387:
+	VDBG(dev, "%s\n", __func__);

which lets us eliminate the debug message from printer_close() altogether.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
Attention

This patch depends on a recent patch from Dan Carpenter:

usb: gadget: function: use after free in printer_close()

 drivers/usb/gadget/function/f_printer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 01e842e1ba2f..d686c7be4fb5 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -11,6 +11,8 @@
  * Copyright (C) 2006 Craig W. Nadler
  */
 
+/* #define VERBOSE_DEBUG */
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
@@ -364,7 +366,8 @@ printer_open(struct inode *inode, struct file *fd)
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	kref_get(&dev->kref);
-	DBG(dev, "printer_open returned %x\n", ret);
+	VDBG(dev, "%s returned %x\n", __func__, ret);
+
 	return ret;
 }
 
@@ -381,7 +384,6 @@ printer_close(struct inode *inode, struct file *fd)
 	dev->printer_status &= ~PRINTER_SELECTED;
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	DBG(dev, "printer_close\n");
 	kref_put(&dev->kref, printer_dev_free);
 
 	return 0;
-- 
2.25.1

Re: [PATCH] usb: gadget: function: Simplify error messaging in printer open/close

[Greg Kroah-Hartman]

On Tue, Nov 22, 2022 at 03:16:03PM +0100, Andrzej Pietrasiewicz wrote:
> Don't issue any messages if printer_open() is successful.
> Also don't issue them if unsuccessful - the error code is propagated
> to the calling layers and should be acted on appropriately there. Just as
> it is with the -ENODEV case.
> 
> For those who really want this message leave an option to compile-in
> with composite framework's VDBG() by uncommenting #define VERBOSE_DEBUG.
> 
> While at it, visually detach the "return ret;" statement.
> 
> Use __func__ instead of explicitly hardcoding the function name. This, in
> turn makes checkpatch issue this for the message in printer_close():
> 
> WARNING: Unnecessary ftrace-like logging - prefer using ftrace
> 54: FILE: drivers/usb/gadget/function/f_printer.c:387:
> +	VDBG(dev, "%s\n", __func__);
> 
> which lets us eliminate the debug message from printer_close() altogether.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
> Attention
> 
> This patch depends on a recent patch from Dan Carpenter:
> 
> usb: gadget: function: use after free in printer_close()
> 
>  drivers/usb/gadget/function/f_printer.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> index 01e842e1ba2f..d686c7be4fb5 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -11,6 +11,8 @@
>   * Copyright (C) 2006 Craig W. Nadler
>   */
>  
> +/* #define VERBOSE_DEBUG */
> +
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
> @@ -364,7 +366,8 @@ printer_open(struct inode *inode, struct file *fd)
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  
>  	kref_get(&dev->kref);
> -	DBG(dev, "printer_open returned %x\n", ret);
> +	VDBG(dev, "%s returned %x\n", __func__, ret);

This is what ftrace is for, please just delete this line entirely.

thanks,

greg k-h

[PATCH] usb: gadget: function: Simplify diagnostic messaging in printer

[Andrzej Pietrasiewicz]

Don't issue messages which can be easily achieved with ftrace.
In case of printer_open() the return code is propagated to other layers
so the user will know about -EBUSY anyway.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
Attention

This patch depends on a recent patch from Dan Carpenter:

usb: gadget: function: use after free in printer_close()

 drivers/usb/gadget/function/f_printer.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 01e842e1ba2f..4903d761a872 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -364,7 +364,7 @@ printer_open(struct inode *inode, struct file *fd)
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	kref_get(&dev->kref);
-	DBG(dev, "printer_open returned %x\n", ret);
+
 	return ret;
 }
 
@@ -381,7 +381,6 @@ printer_close(struct inode *inode, struct file *fd)
 	dev->printer_status &= ~PRINTER_SELECTED;
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	DBG(dev, "printer_close\n");
 	kref_put(&dev->kref, printer_dev_free);
 
 	return 0;
@@ -848,8 +847,6 @@ static void printer_reset_interface(struct printer_dev *dev)
 	if (dev->interface < 0)
 		return;
 
-	DBG(dev, "%s\n", __func__);
-
 	if (dev->in_ep->desc)
 		usb_ep_disable(dev->in_ep);
 
@@ -887,8 +884,6 @@ static void printer_soft_reset(struct printer_dev *dev)
 {
 	struct usb_request	*req;
 
-	INFO(dev, "Received Printer Reset Request\n");
-
 	if (usb_ep_disable(dev->in_ep))
 		DBG(dev, "Failed to disable USB in_ep\n");
 	if (usb_ep_disable(dev->out_ep))
@@ -1185,8 +1180,6 @@ static void printer_func_disable(struct usb_function *f)
 {
 	struct printer_dev *dev = func_to_printer(f);
 
-	DBG(dev, "%s\n", __func__);
-
 	printer_reset_interface(dev);
 }
 
-- 
2.25.1