netdev - Re: [PATCH net 06/10] iavf: prevent accidental free of filter structure

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] [day] [month] [year] [list]

Message-ID: <20211117063708.ekrxtv7e6jn5thvp@p1>
Date:   Wed, 17 Nov 2021 07:37:08 +0100
From:   Stefan Assmann <sassmann@...hat.com>
To:     "Keller, Jacob E" <jacob.e.keller@...el.com>
Cc:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Brelinski, Tony" <tony.brelinski@...el.com>
Subject: Re: [PATCH net 06/10] iavf: prevent accidental free of filter
 structure

On 2021-11-16 20:18, Keller, Jacob E wrote:
> On 11/15/2021 11:24 PM, Stefan Assmann wrote:
> > On 2021-11-15 15:59, Tony Nguyen wrote:
> >> From: Jacob Keller <jacob.e.keller@...el.com>
> >>
> >> In iavf_config_clsflower, the filter structure could be accidentally
> >> released at the end, if iavf_parse_cls_flower or iavf_handle_tclass ever
> >> return a non-zero but positive value.
> >>
> >> In this case, the function continues through to the end, and will call
> >> kfree() on the filter structure even though it has been added to the
> >> linked list.
> >>
> >> This can actually happen because iavf_parse_cls_flower will return
> >> a positive IAVF_ERR_CONFIG value instead of the traditional negative
> >> error codes.
> > 
> > Hi Jacob,
> > 
> > where exactly does this happen?
> > Looking at iavf_parse_cls_flower() I see all returns of IAVF_ERR_CONFIG
> > as "return IAVF_ERR_CONFIG;" while IAVF_ERR_CONFIG is defined as
> >         IAVF_ERR_CONFIG                         = -4,
> > 
> > I'm not opposed to this change, just wondering what's going on.
> > 
> >   Stefan
> > 
> 
> Heh.
> 
> I don't have memory of the full context for the original work. We've
> been going through and trying to pull in fixes that we've done for our
> out-of-tree driver and get everything upstream.
> 
> At first I thought this might be because of some history where these
> values used to be positive in the out-of-tree history at some point...
> But I think this wasn't true. It is possible that some other flow
> accidentally sends a positive value, but I've long since lost memory of
> if I had an example of that. You're correct that IAVF_ERR_CONFIG is (and
> has been in both upstream and out-of-tree code since its inception)
> negative.
> 
> I don't think this change is harmful, but I think you're right in
> pointing out the description isn't really valid.
> 
> I'm happy to re-write this commit message for clarity.
> 
> I do think switching to "if (err)" is more idiomatic and the correct
> thing to do.

Great feedback thanks, I totally agree.

  Stefan