netdev - Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}

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]

Message-ID: <d8417f98-0896-25d0-e72d-dcf111011129@gmail.com>
Date:   Thu, 29 Oct 2020 20:46:59 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Oliver Neukum <oneukum@...e.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev <netdev@...r.kernel.org>, linux-usb@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd()
 to use new API


On 29/10/20 6:52 pm, Anant Thazhemadam wrote:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
> and thus consider potential partial reads/writes being done to 
> be perfectly valid.
> Quite a few callers of usbnet_{read|write}_cmd() don't enforce
> checking for partial reads/writes into account either, automatically
> assuming that a complete read/write occurs.
>
> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
> reads and writes.
> Using the new APIs also relaxes the return value checking that must
> be done after usbnet_{read|write}_cmd() is called.
>
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com> <mailto:anant.thazhemadam@...il.com>
> ---
> Changes in v2:
> 	* Fix build error
>
> This patch has been compile and build tested with a .config file that
> was generated using make allyesconfig, and the build error has been 
> fixed.
> Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
> and would appreciate it if someone could do that.
>
>  drivers/net/usb/usbnet.c | 52 ++++++++--------------------------------
>  1 file changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index bf6c58240bd4..2f7c7b7f4047 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
>  static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
>  			     u16 value, u16 index, void *data, u16 size)
>  {
> -	void *buf = NULL;
> -	int err = -ENOMEM;
>  
>  	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
>  		   " value=0x%04x index=0x%04x size=%d\n",
>  		   cmd, reqtype, value, index, size);
>  
> -	if (size) {
> -		buf = kmalloc(size, GFP_KERNEL);
> -		if (!buf)
> -			goto out;
> -	}
> -
> -	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -			      cmd, reqtype, value, index, buf, size,
> -			      USB_CTRL_GET_TIMEOUT);
> -	if (err > 0 && err <= size) {
> -        if (data)
> -            memcpy(data, buf, err);
> -        else
> -            netdev_dbg(dev->net,
> -                "Huh? Data requested but thrown away.\n");
> -    }
> -	kfree(buf);
> -out:
> -	return err;
> +	return usb_control_msg_recv(dev->udev, 0,
> +			      cmd, reqtype, value, index, data, size,
> +			      USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
>  			      u16 value, u16 index, const void *data,
>  			      u16 size)
>  {
> -	void *buf = NULL;
> -	int err = -ENOMEM;
> -
>  	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
>  		   " value=0x%04x index=0x%04x size=%d\n",
>  		   cmd, reqtype, value, index, size);
>  
> -	if (data) {
> -		buf = kmemdup(data, size, GFP_KERNEL);
> -		if (!buf)
> -			goto out;
> -	} else {
> -        if (size) {
> -            WARN_ON_ONCE(1);
> -            err = -EINVAL;
> -            goto out;
> -        }
> -    }
> -
> -	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> -			      cmd, reqtype, value, index, buf, size,
> -			      USB_CTRL_SET_TIMEOUT);
> -	kfree(buf);
> +	if (size && !data) {
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
>  
> -out:
> -	return err;
> +	return usb_control_msg_send(dev->udev, 0,
> +			cmd, reqtype, value, index, data, size,
> +			USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  /*

I had a v2 prepared and ready but was told to wait for a week before sending it in,
since usb_control_msg_{send|recv}() that were being used were not present in the
networking tree at the time, and all the trees would be converged by then.
So, just to be on the safer side, I waited for two weeks.
I checked the net tree, and found the APIs there too (defined in usb.h).

However the build seems to fail here,
    https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/

I'm not entirely sure at this point why this is happening, and would appreciate it if
someone could take the time to tell me if and how this might be an issue with my
patch.

Thanks,
Anant