linux-kernel - Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw

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:   Fri, 27 Aug 2021 11:22:57 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     David Laight <David.Laight@...LAB.COM>,
        'Dan Carpenter' <dan.carpenter@...cle.com>
Cc:     "Larry.Finger@...inger.net" <Larry.Finger@...inger.net>,
        "phil@...lpotter.co.uk" <phil@...lpotter.co.uk>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "straube.linux@...il.com" <straube.linux@...il.com>,
        "fmdefrancesco@...il.com" <fmdefrancesco@...il.com>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8

On 8/27/21 11:14 AM, David Laight wrote:
> From: Dan Carpenter
>> Sent: 26 August 2021 12:21
> ...
>> > > > ...
>> > > > > +		len += snprintf(page + len, count - len,
>> > > > > "rtw_read8(0x%x)=0x%x\n",
>> > > > > +				proc_get_read_addr, (u8) tmp);
>> > > >
>> > > > That is broken.
>> > > >
>> > >
>> > > Don't get it, sorry. Previous code did exactly the same thing, but
>> > > didn't check if read() was successful.
>> >
>> > Look up the return value of snprintf().
>> >
>> 
>> It's hard to understand what you are saying.  I think you are confusing
>> libc snprintf with the kernel snprintf?  In libc the snprintf function
>> can return negatives but in the kernel it cannot.  This is not going
>> to change.  Any code which checks for negative snprintf returns in the
>> kernel is wrong and should be fixed.
>> 
>> Anyway, the code works fine.  snprintf here is going to return a number
>> between 18-32 range.  It's not going to overflow the PAGE_SIZE buffer.
> 
> IIRC it is also in a loop ...
> 
> Maybe, but the idiom is just broken.
> Largely the result of snprintf() is never the value you are looking
> for and should be ignored.
> 

AFAIK, snprintf return value is largely used in sysfs at least.

$ grep -Ir "= snprintf" | wc -l
1200

Anyway, I will convert this dead code to sysfs interface and maybe 
snprintf will go away...



Thank you for your help and comments


> Userspace fprintf() is even worse.
> If you care about the write failing you need to call fflush()
> and then ferror() (typically before fclose()).
> 
> Fortunately I've never seen a 'must check' attribute on it.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 


With regards,
Pavel Skripkin