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  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, 16 Sep 2020 09:19:46 +0300
From:   Petko Manolov <petkan@...leusys.com>
To:     Anant Thazhemadam <anant.thazhemadam@...il.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs
 on failed register reads

On 20-09-16 10:35:40, Anant Thazhemadam wrote:
> get_registers() copies whatever memory is written by the
> usb_control_msg() call even if the underlying urb call ends up failing.

Not true, memcpy() is only called if "ret" is positive.

> If get_registers() fails, or ends up reading 0 bytes, meaningless and junk 
> register values would end up being copied over (and eventually read by the 
> driver), and since most of the callers of get_registers() don't check the 
> return values of get_registers() either, this would go unnoticed.

usb_control_msg() returns negative on error (look up usb_internal_control_msg() 
to see for yourself) so it does not go unnoticed.  If for some reason it return 
zero, nothing is copied.  Also, if usb transfer fail no register values are 
being copied anywhere.

Your patch also allows for memcpy() to be called with 'size' either zero or 
greater than the allocated buffer size. Please, look at the code carefully.

> It might be a better idea to try and mirror the PCI master abort
> termination and set memory to 0xFFs instead in such cases.

I wasn't aware drivers are now responsible for filling up the memory with 
anything.  Does not sound like a good idea to me.

> Fixes: https://syzkaller.appspot.com/bug?extid=abbc768b560c84d92fd3
> Reported-by: syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com
> Tested-by: syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com>

Well, NACK from me.


cheers,
Petko


> ---
>  drivers/net/usb/rtl8150.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 733f120c852b..04fca7bfcbcb 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -162,8 +162,13 @@ static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
>  	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
>  			      RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
>  			      indx, 0, buf, size, 500);
> -	if (ret > 0 && ret <= size)
> +
> +	if (ret < 0)
> +		memset(data, 0xff, size);
> +
> +	else
>  		memcpy(data, buf, ret);
> +
>  	kfree(buf);
>  	return ret;
>  }
> @@ -276,7 +281,7 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 indx, u16 reg)
>  
>  static inline void set_ethernet_addr(rtl8150_t * dev)
>  {
> -	u8 node_id[6];
> +	u8 node_id[6] = {0};
>  
>  	get_registers(dev, IDR, sizeof(node_id), node_id);
>  	memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> -- 
> 2.25.1
> 
> 

Powered by blists - more mailing lists