linux-kernel - Re: [PATCH] net: ax88179: add proper error handling of usb read errors

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]

Message-ID: <CAAZOf27Q-QQ51pGO1gFETNR0ASg6zmxF4HUFUVn77oL3Cs7LEg@mail.gmail.com>
Date:   Sat, 16 Apr 2022 14:49:37 +0300
From:   David Kahurani <k.kahurani@...il.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     netdev@...r.kernel.org,
        syzbot <syzbot+d3dbdf31fbe9d8f5f311@...kaller.appspotmail.com>,
        davem@...emloft.net, jgg@...pe.ca, kuba@...nel.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        Phillip Potter <phil@...lpotter.co.uk>,
        syzkaller-bugs@...glegroups.com, arnd@...db.de,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read errors

On Sat, Apr 16, 2022 at 2:10 PM Pavel Skripkin <paskripkin@...il.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@...il.com>
> > Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@...kaller.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