netdev - Re: [PATCH v2 net] net: hso: check for allocation failure in hso_create_bulk_serial

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 May 2021 10:07:25 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Oliver Neukum <oneukum@...e.com>,
        Anirudh Rayabharam <mail@...rudhrb.com>,
        Zheng Yongjun <zhengyongjun3@...wei.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        Rustam Kovhaev <rkovhaev@...il.com>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH v2 net] net: hso: check for allocation failure in
 hso_create_bulk_serial_device()

On Fri, May 14, 2021 at 05:24:48PM +0300, Dan Carpenter wrote:
> In current kernels, small allocations never actually fail so this
> patch shouldn't affect runtime.
> 
> Originally this error handling code written with the idea that if
> the "serial->tiocmget" allocation failed, then we would continue
> operating instead of bailing out early.  But in later years we added
> an unchecked dereference on the next line.
> 
> 	serial->tiocmget->serial_state_notification = kzalloc();
>         ^^^^^^^^^^^^^^^^^^
> 
> Since these allocations are never going fail in real life, this is
> mostly a philosophical debate, but I think bailing out early is the
> correct behavior that the user would want.  And generally it's safer to
> bail as soon an error happens.
> 
> Fixes: af0de1303c4e ("usb: hso: obey DMA rules in tiocmget")
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
> v2: Do more extensive clean up.  As Johan pointed out the comments and
> later NULL checks can be removed.
> 
>  drivers/net/usb/hso.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 3ef4b2841402..260f850d69eb 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -2618,29 +2618,28 @@ static struct hso_device *hso_create_bulk_serial_device(
>  		num_urbs = 2;
>  		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
>  					   GFP_KERNEL);
> +		if (!serial->tiocmget)
> +			goto exit;
>  		serial->tiocmget->serial_state_notification
>  			= kzalloc(sizeof(struct hso_serial_state_notification),
>  					   GFP_KERNEL);
> -		/* it isn't going to break our heart if serial->tiocmget
> -		 *  allocation fails don't bother checking this.
> -		 */
> -		if (serial->tiocmget && serial->tiocmget->serial_state_notification) {
> -			tiocmget = serial->tiocmget;
> -			tiocmget->endp = hso_get_ep(interface,
> -						    USB_ENDPOINT_XFER_INT,
> -						    USB_DIR_IN);
> -			if (!tiocmget->endp) {
> -				dev_err(&interface->dev, "Failed to find INT IN ep\n");
> -				goto exit;
> -			}
> -
> -			tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL);
> -			if (tiocmget->urb) {
> -				mutex_init(&tiocmget->mutex);
> -				init_waitqueue_head(&tiocmget->waitq);
> -			} else
> -				hso_free_tiomget(serial);
> +		if (!serial->tiocmget->serial_state_notification)
> +			goto exit;
> +		tiocmget = serial->tiocmget;
> +		tiocmget->endp = hso_get_ep(interface,
> +					    USB_ENDPOINT_XFER_INT,
> +					    USB_DIR_IN);
> +		if (!tiocmget->endp) {
> +			dev_err(&interface->dev, "Failed to find INT IN ep\n");
> +			goto exit;
>  		}
> +
> +		tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (tiocmget->urb) {
> +			mutex_init(&tiocmget->mutex);
> +			init_waitqueue_head(&tiocmget->waitq);
> +		} else
> +			hso_free_tiomget(serial);

This should probably be changed to bail out on allocation errors as well
now but that can be done as a follow-up. Either way:

Reviewed-by: Johan Hovold <johan@...nel.org>

>  	}
>  	else
>  		num_urbs = 1;

Johan