Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 16 Sep 2020 09:39:25 +0300
From:   Petko Manolov <petkan@nucleusys.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Anant Thazhemadam <anant.thazhemadam@gmail.com>,
        linux-kernel-mentees@lists.linuxfoundation.org,
        syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-usb@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs
 on failed register reads
Message-ID: <20200916063925.GC38262@p310>
References: <20200916050540.15290-1-anant.thazhemadam@gmail.com>
 <20200916062227.GD142621@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200916062227.GD142621@kroah.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 20-09-16 08:22:27, Greg KH wrote:
> On Wed, Sep 16, 2020 at 10:35:40AM +0530, 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.
> > 
> > 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.
> > 
> > It might be a better idea to try and mirror the PCI master abort
> > termination and set memory to 0xFFs instead in such cases.
> 
> It would be better to use this new api call instead of
> usb_control_msg():
> 	https://lore.kernel.org/r/20200914153756.3412156-1-gregkh@linuxfoundation.org

Heh, wasn't aware of the new api.

> How about porting this patch to run on top of that series instead?  That 
> should make this logic much simpler.

I'll need to check if in this case 'size' is the right amount of bytes expected 
and not an upper limit.  Then i'll convert it to the new api.


cheers,
Petko


> > Fixes: https://syzkaller.appspot.com/bug?extid=abbc768b560c84d92fd3
> > Reported-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com
> > Tested-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> > ---
> >  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};
> 
> This should not be needed to be done.
> 
> thanks,
> 
> greg k-h
>