usb: gadget: function: use after free in printer_close()
Subsystems (2)
PATCH 1/1 usb: gadget: function: use after free in printer_close()
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.
---
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(-)
drivers/usb/gadget/function/f_printer.c +1 -1
@@ -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 ; }
Signature · 1 line
2.35.1
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:
Quoted 6 lines
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 <[email protected]>
Quoted 21 lines
---
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(-)drivers/usb/gadget/function/f_printer.c +1
-1@@ -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 ; }
Quoted 9 lines · 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.
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.
Quoted 10 lines
W dniu 21.11.2022 o 15:44, Dan Carpenter pisze:
Quoted 6 lines
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 <[email protected]>
Thanks!
regards,
dan carpenter
Hi Dan,
W dniu 22.11.2022 o 08:00, Dan Carpenter pisze:
Quoted 6 lines · Andrzej Pietrasiewicz
Quoted 5 lines
Hi Dan,
I'm fine with either symmetrically removing the
DBG()from "printer_open()" or with this version of the patch.
Quoted 9 lines
Quoted 4 lines
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
Quoted 19 lines
I obviously considered this as v1 but thought deleting was better. I still do. :) But it's not worth spending time on.
Quoted 10 lines
W dniu 21.11.2022 o 15:44, Dan Carpenter pisze:
Quoted 6 lines
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 <[email protected]>Thanks!
regards,
dan carpenter
PATCH 1/1 usb: gadget: function: Simplify error messaging in printer open/close
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.
---
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(-)
drivers/usb/gadget/function/f_printer.c +4 -2
@@ -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 ;
Signature · 1 line
2.25.1
Greg Kroah-Hartman
maintainerRe: [PATCH] usb: gadget: function: Simplify error messaging in printer open/close
Quoted 44 lines · 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-ENODEVcase.For those who really want this message leave an option to compile-in with composite framework's
VDBG()by uncommenting #defineVERBOSE_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 inprinter_close():WARNING: Unnecessary ftrace-like logging - prefer using ftrace 54: FILE:
drivers/usb/gadget/function/f_printer.c:387:(quoted diff) +1
-0+ VDBG(dev, "%s\n", __func__);which lets us eliminate the debug message from
printer_close()altogether.Signed-off-by Andrzej Pietrasiewicz <[email protected]>---
AttentionThis 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(-)drivers/usb/gadget/function/f_printer.c +2
-0@@ -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>
drivers/usb/gadget/function/f_printer.c printer_open(struct inode *inode, struct file *fd) · line 369
Quoted 5 lines
drivers/usb/gadget/function/f_printer.c +1
-1@@ -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 1/1 usb: gadget: function: Simplify diagnostic messaging in printer
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.
---
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(-)
drivers/usb/gadget/function/f_printer.c +1 -8
@@ -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); }
Signature · 1 line
2.25.1