From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.0 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 893E1C43461 for ; Wed, 16 Sep 2020 07:02:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 11D05206BE for ; Wed, 16 Sep 2020 07:02:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=nucleusys.com header.i=@nucleusys.com header.b="GjClFFip" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726336AbgIPHCu (ORCPT ); Wed, 16 Sep 2020 03:02:50 -0400 Received: from lan.nucleusys.com ([92.247.61.126]:56168 "EHLO zztop.nucleusys.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726128AbgIPHCq (ORCPT ); Wed, 16 Sep 2020 03:02:46 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=nucleusys.com; s=x; h=In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=8fk+I4ZemBY5OogXFdLgsK51XfiRHsT4RBnrN+8HFMs=; b=GjClFFipRMYmH+d63qEmRBuFZk tbSOhuRA3TDineS+sjSW56nDqjshXWGsvcDP7vVm93n+woR2YO9knE+1tCYpthCNoAoMySrQDJ15H UGW0yStPQHrGcI6Z46aIBOKaZd9vaDN5eAKF15c9OL9kuxkUX4XqF4zKwnEytCz3OKwU=; Received: from 78-83-68-78.spectrumnet.bg ([78.83.68.78] helo=p310) by zztop.nucleusys.com with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kIQnC-00058i-Qu; Wed, 16 Sep 2020 09:19:47 +0300 Date: Wed, 16 Sep 2020 09:19:46 +0300 From: Petko Manolov To: Anant Thazhemadam Cc: linux-kernel-mentees@lists.linuxfoundation.org, syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com, "David S. Miller" , Jakub Kicinski , 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: <20200916061946.GA38262@p310> References: <20200916050540.15290-1-anant.thazhemadam@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200916050540.15290-1-anant.thazhemadam@gmail.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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@syzkaller.appspotmail.com > Tested-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com > Signed-off-by: Anant Thazhemadam 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 > >