linux-kernel - Re: [PATCH v3 1/7] staging: r8188eu: remove unused member free_bss

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: <20220420173926.GE2969@kadam>
Date:   Wed, 20 Apr 2022 20:39:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jaehee Park <jhpark1013@...il.com>
Cc:     Pavel Skripkin <paskripkin@...il.com>, Larry.Finger@...inger.net,
        phil@...lpotter.co.uk, gregkh@...uxfoundation.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev
Subject: Re: [PATCH v3 1/7] staging: r8188eu: remove unused member
 free_bss_buf

On Wed, Apr 20, 2022 at 10:48:34AM -0400, Jaehee Park wrote:
> On Wed, Apr 20, 2022 at 02:55:34PM +0300, Dan Carpenter wrote:
> > As a kernel community, I don't think we are pro-active enough about
> > preventing bugs.  One of my co-workers and I have started a bi weekly
> > phone call to look at CVEs from the previous week and discuss how they
> > could have been prevented or detected earlier.  Of course, my solutions
> > are always centered around static analysis because that's my deal but
> > some bugs are caused by process issues, or could be detected with better
> > testing.
> > 
> > In this case there were two bugs proposed bugs.
> > 
> > 1) The memory leak that Fabio noticed.  Smatch is bad at detecting
> >    memory leaks.  It's a hard problem, because in this case it's
> >    across function boundaries.
> > 
> > Fabio caught the leak.  I don't know if I would have.
> > 
> > 2) The NULL dereference.
> > 
> >    The "&pnetwork->list" expression is not a dereference so this is also
> >    a cross function thing.  I thought I used to have an unpublished check
> >    for bogus addresses like that where pnetwork is NULL.
> > 
> >    Another is idea is that when you have pnetwork++ and it's a NULL
> >    pointer then print an error message.  Or even potentially NULL.
> >    There are various heuristics to use which mean that "A reasonable
> >    person would think this could be NULL".
> > 
> >    Or another idea would be that we could test patches.  Right now we
> >    don't really have a way to test these.  But, of course, we wish we
> >    did.
> > 
> > It's not super likely that we would have committed the NULL deref
> > patch.  I would have caught that one if you didn't and Fabio likely
> > would have as well.  But I like to remove the human error whenever I
> > can.
> > 
> > regards,
> > dan carpenter
> 
> I'm sorry about the NULL dereference. I wasn't sure about pnetwork
> and I should've asked. I wanted to ask why pbuf should be removed
> when it was being used for pnetwork but ended up not asking and
> sent a faulty patch. Sorry again and thank you for explaining the
> errors. I will be more careful about memory leaks and dereferencing
> errors. Are there checks that I can run to detect these?

I was going to suggest that you could write a check yourself, but then
when I looked at it it became quite complicated.  :P

How about I write the check and send you the output tomorrow?

regards,
dan carpenter