usb-serial: fix mos_parport refcount imbalance on error path

usb-serial: fix mos_parport refcount imbalance on error path

[Lin Yi]

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;

usb-serial: fix mos_parport refcount imbalance on error path

[Greg Kroah-Hartman]

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

usb-serial: fix mos_parport refcount imbalance on error path

[Johan Hovold]

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

usb-serial: fix mos_parport refcount imbalance on error path

[Jian Liu/Gmail]

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