Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220416074817.571160-1-k.kahurani@gmail.com> <65c52645-26e8-ff2b-86dc-b5dd697317f9@gmail.com>
In-Reply-To: <65c52645-26e8-ff2b-86dc-b5dd697317f9@gmail.com>
From:   David Kahurani <k.kahurani@gmail.com>
Date:   Sat, 16 Apr 2022 14:49:37 +0300
Message-ID: <CAAZOf27Q-QQ51pGO1gFETNR0ASg6zmxF4HUFUVn77oL3Cs7LEg@mail.gmail.com>
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read errors
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     netdev@vger.kernel.org,
        syzbot <syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com>,
        davem@davemloft.net, jgg@ziepe.ca, kuba@kernel.org,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        Phillip Potter <phil@philpotter.co.uk>,
        syzkaller-bugs@googlegroups.com, arnd@arndb.de,
        Dan Carpenter <dan.carpenter@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Apr 16, 2022 at 2:10 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Hi David,

Hi Pavel.

>
> one more small comment
>
> On 4/16/22 10:48, David Kahurani wrote:
> > Reads that are lesser than the requested size lead to uninit-value bugs.
> > In this particular case a variable which was supposed to be initialized
> > after a read is left uninitialized after a partial read.
> >
> > Qualify such reads as errors and handle them correctly and while at it
> > convert the reader functions to return zero on success for easier error
> > handling.
> >
> > Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> > gigabit ethernet adapter driver")
> > Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> > Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> > ---
>
> [code snip]
>
> > @@ -1295,6 +1439,7 @@ static int ax88179_led_setting(struct usbnet *dev)
> >   static void ax88179_get_mac_addr(struct usbnet *dev)
> >   {
> >       u8 mac[ETH_ALEN];
> > +     int ret;
> >
> >       memset(mac, 0, sizeof(mac));
> >
> > @@ -1303,8 +1448,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
> >               netif_dbg(dev, ifup, dev->net,
> >                         "MAC address read from device tree");
> >       } else {
> > -             ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> > -                              ETH_ALEN, mac);
> > +             ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> > +                                    ETH_ALEN, mac);
> > +
> > +             if (ret)
> > +                     netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> > +
> >               netif_dbg(dev, ifup, dev->net,
> >                         "MAC address read from ASIX chip");
> >       }
>
>
> This message sequence is confusing.
>
> In case of ax88179_read_cmd() failure mac read from device actually
> failed, but message says, that it was successfully finished.

I suppose the code should return in case of an error that way the next
message does not get executed.

Thanks for the review! Will fix it and the other issue in the next version.

>
>
>
>
>
> With regards,
> Pavel Skripkin