netdev - Re: [PATCH -next] ieee802154: atusb: move to new USB API

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:   Wed, 5 Jan 2022 21:27:43 +0100
From:   Stefan Schmidt <stefan@...enfreihafen.org>
To:     Pavel Skripkin <paskripkin@...il.com>, alex.aring@...il.com,
        davem@...emloft.net, kuba@...nel.org, linux-wpan@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] ieee802154: atusb: move to new USB API


Hello.

On 05.01.22 15:49, Pavel Skripkin wrote:
> Old USB API is prone to uninit value bugs if error handling is not
> correct. Let's move atusb to use new USB API to
> 
> 	1) Make code more simple, since new API does not require memory
> 	   to be allocates via kmalloc()
> 
> 	2) Defend driver from usb-related uninit value bugs.
> 
> 	3) Make code more modern and simple
> 
> This patch removes atusb usb wrappers as Greg suggested [0], this will make
> code more obvious and easier to understand over time, and replaces old
> API calls with new ones.
> 
> Also this patch adds and updates usb related error handling to prevent
> possible uninit value bugs in future
> 
> Link: https://lore.kernel.org/all/YdL0GPxy4TdGDzOO@kroah.com/ [0]
> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> ---
> 
> Only build tested.

Gave it a first quick run on real hardware here. Besides one small bug 
(see below) it looked good.

Will give it a bit more testing over the next days.



> @@ -881,14 +819,27 @@ static int atusb_get_and_conf_chip(struct atusb *atusb)
>   	u8 man_id_0, man_id_1, part_num, version_num;
>   	const char *chip;
>   	struct ieee802154_hw *hw = atusb->hw;
> +	int ret;
>   
> -	man_id_0 = atusb_read_reg(atusb, RG_MAN_ID_0);
> -	man_id_1 = atusb_read_reg(atusb, RG_MAN_ID_1);
> -	part_num = atusb_read_reg(atusb, RG_PART_NUM);
> -	version_num = atusb_read_reg(atusb, RG_VERSION_NUM);
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_MAN_ID_0, &man_id_0, 1, 1000, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
>   
> -	if (atusb->err)
> -		return atusb->err;
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_MAN_ID_1, &man_id_1, 1, 1000, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_PART_NUM, &atusb, 1, 1000, GFP_KERNEL);

This needs to be written to &part_num and not &atusb.

Pretty nice for a first blind try without hardware. Thanks.

Will let you know if I find anything else from testing.

regards
Stefan Schmidt