| Message ID | 20190320022916.GA5199@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb-serial: fix mos_parport refcount imbalance on error path | expand |
Hi all, We might find a security issue in function 'write_parport_reg_nonbloc'(for source codes, please refer to https://github.com/torvalds/linux/blob/master/drivers/usb/serial/mos7720.c), which is logically an unbalanced reference count vulnerability. Let us check ref_count 'mos_parport->ref_count' of kernel object 'mos_parport' in the function. In 'Line 369: kref_get(&mos_parport->ref_count)' and 'Line 370: urbtrack->mos_parport = mos_parport', the ref_count has been increased because of a new variable 'urbtrack->mos_parport' pointing to 'mos_parport', that make sense to balance ref_count. However, at 'Line 373 and Line 379: kfree(urbtrack)', variable 'urbtrack' has been freed before a return, which will make 'urbtrack->mos_parport' freed, too. But, a decreasing of 'mos_parport->ref_count' is missing in the current kernel version, which will bring an unbalanced reference count. We have wrote a patch showed in Lin's email. The idea is simple. We added a calling of 'kref_put(&mos_parport->ref_count, destroy_mos_parport)' below Line 373 and Line 379. Currently, we are not sure if this is really a vulnerability. If you are familiar with codes, please help to verify it. Thanks. Jian 2019-03-20 10:29 GMT+08:00, Lin Yi <teroincn@163.com>: > write_parport_ref_nonblock increase mos_parport refcount without > decrease it when return -ENOMEM code, so need a decrement before function > return -ENOMEM. > > Signed-off-by: Lin Yi <teroincn@163.com> > --- > drivers/usb/serial/mos7720.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c > index fc52ac7..6abb335 100644 > --- a/drivers/usb/serial/mos7720.c > +++ b/drivers/usb/serial/mos7720.c > @@ -371,12 +371,14 @@ static int write_parport_reg_nonblock(struct > mos7715_parport *mos_parport, > urbtrack->urb = usb_alloc_urb(0, GFP_ATOMIC); > if (!urbtrack->urb) { > kfree(urbtrack); > + kref_put(&mos_parport->ref_count, destroy_mos_parport); > return -ENOMEM; > } > urbtrack->setup = kmalloc(sizeof(*urbtrack->setup), GFP_ATOMIC); > if (!urbtrack->setup) { > usb_free_urb(urbtrack->urb); > kfree(urbtrack); > + kref_put(&mos_parport->ref_count, destroy_mos_parport); > return -ENOMEM; > } > urbtrack->setup->bRequestType = (__u8)0x40; > -- > 1.9.1 > > >
On Wed, Mar 20, 2019 at 10:29:16AM +0800, Lin Yi wrote: > write_parport_ref_nonblock increase mos_parport refcount without > decrease it when return -ENOMEM code, so need a decrement before function > return -ENOMEM. Good catch! Your patch looks good, but wouldn't it be better to move the kref_get() (and mos_parport assignment) to the end of the urbtrack initialisation instead? That way you wouldn't have to add any kref_puts() at all. > Signed-off-by: Lin Yi <teroincn@163.com> > --- > drivers/usb/serial/mos7720.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c > index fc52ac7..6abb335 100644 > --- a/drivers/usb/serial/mos7720.c > +++ b/drivers/usb/serial/mos7720.c > @@ -371,12 +371,14 @@ static int write_parport_reg_nonblock(struct mos7715_parport *mos_parport, > urbtrack->urb = usb_alloc_urb(0, GFP_ATOMIC); > if (!urbtrack->urb) { > kfree(urbtrack); > + kref_put(&mos_parport->ref_count, destroy_mos_parport); > return -ENOMEM; > } > urbtrack->setup = kmalloc(sizeof(*urbtrack->setup), GFP_ATOMIC); > if (!urbtrack->setup) { > usb_free_urb(urbtrack->urb); > kfree(urbtrack); > + kref_put(&mos_parport->ref_count, destroy_mos_parport); > return -ENOMEM; > } > urbtrack->setup->bRequestType = (__u8)0x40; Thanks, Johan
On Wed, Mar 20, 2019 at 04:59:02PM +0800, Jian Liu/Gmail wrote: > Hi all, > > We might find a security issue in function > 'write_parport_reg_nonbloc'(for source codes, please refer to > https://github.com/torvalds/linux/blob/master/drivers/usb/serial/mos7720.c), > which is logically an unbalanced reference count vulnerability. > > Let us check ref_count 'mos_parport->ref_count' of kernel object > 'mos_parport' in the function. In 'Line 369: > kref_get(&mos_parport->ref_count)' and 'Line 370: > urbtrack->mos_parport = mos_parport', the ref_count has been increased > because of a new variable 'urbtrack->mos_parport' pointing to > 'mos_parport', that make sense to balance ref_count. However, at 'Line > 373 and Line 379: kfree(urbtrack)', variable 'urbtrack' has been freed > before a return, which will make 'urbtrack->mos_parport' freed, too. > But, a decreasing of 'mos_parport->ref_count' is missing in the > current kernel version, which will bring an unbalanced reference > count. > > We have wrote a patch showed in Lin's email. The idea is simple. We > added a calling of 'kref_put(&mos_parport->ref_count, > destroy_mos_parport)' below Line 373 and Line 379. Currently, we are > not sure if this is really a vulnerability. If you are familiar with > codes, please help to verify it. Yes, this is a bugfix, but not really a "vulnerability" as it is almost impossible to keep hitting a memory allocation failure enough times to cause any problems here. The most that can happen is that the reference really is not dropped, and memory leaks. Not a big deal, if memory is failing to be allocated, your system has bigger issues at the moment :) So it's just a nice bugfix, for a failure mode that is pretty much impossible to ever hit in the "wild". thanks, greg k-h
diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c index fc52ac7..6abb335 100644 --- a/drivers/usb/serial/mos7720.c +++ b/drivers/usb/serial/mos7720.c @@ -371,12 +371,14 @@ static int write_parport_reg_nonblock(struct mos7715_parport *mos_parport, urbtrack->urb = usb_alloc_urb(0, GFP_ATOMIC); if (!urbtrack->urb) { kfree(urbtrack); + kref_put(&mos_parport->ref_count, destroy_mos_parport); return -ENOMEM; } urbtrack->setup = kmalloc(sizeof(*urbtrack->setup), GFP_ATOMIC); if (!urbtrack->setup) { usb_free_urb(urbtrack->urb); kfree(urbtrack); + kref_put(&mos_parport->ref_count, destroy_mos_parport); return -ENOMEM; } urbtrack->setup->bRequestType = (__u8)0x40;
write_parport_ref_nonblock increase mos_parport refcount without decrease it when return -ENOMEM code, so need a decrement before function return -ENOMEM. Signed-off-by: Lin Yi <teroincn@163.com> --- drivers/usb/serial/mos7720.c | 2 ++ 1 file changed, 2 insertions(+)