netdev - 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]

Date:   Thu, 14 Apr 2022 11:21:15 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     David Kahurani <k.kahurani@...il.com>, 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,
        Pavel Skripkin <paskripkin@...il.com>
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read
 errors

On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
> 
> 
> On 13.04.22 17:32, Dan Carpenter wrote:
> >
> > Bug: buffer partially filled.  Information leak.
> >
> > If you return the bytes then the only correct way to write error
> > handling is:
> >
> > 	if (ret < 0)
> > 		return ret;
> > 	if (ret != size)
> > 		return -EIO;
> >
> You have to make up your mind on whether you ever need to read
> answer of a length not known before you try it. The alternative of
> passing a pointer to an integer for length is worse.

How is it worse?  Can you give an example, so I will write a static
checker rule for it?

There used to be more APIs that consistently caused bug after bug where
we mixed positives success values with negative error codes.  We
converted some bad offenders to return the positive as a parameter
and I was really happy about that.

Another example I used to see a lot is request_irq() saved to an
unsigned.  These days I think GCC warns about that?  Maybe the build
bots get to it before I do.

regards,
dan carpenter