netdev - Re: [PATCH] mctp: fix use after free

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: <6590666e-524d-51c3-0859-f8bf0c43c5ca@redhat.com>
Date:   Mon, 14 Feb 2022 18:16:44 -0800
From:   Tom Rix <trix@...hat.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>, matt@...econstruct.com.au,
        davem@...emloft.net, kuba@...nel.org, nathan@...nel.org,
        ndesaulniers@...gle.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH] mctp: fix use after free


On 2/14/22 4:44 PM, Jeremy Kerr wrote:
> Hi Tom,
>
>> Clang static analysis reports this problem
>> route.c:425:4: warning: Use of memory after it is freed
>>    trace_mctp_key_acquire(key);
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> When mctp_key_add() fails, key is freed but then is later
>> used in trace_mctp_key_acquire().  Add an else statement
>> to use the key only when mctp_key_add() is successful.
> Looks good to me, thanks for the fix.
>
> However, the Fixes tag will need an update; at the point of
> 4a992bbd3650 ("mctp: Implement message fragmentation"), there was no
> use of 'key' after the kfree() there.
>
> Instead, this is the hunk that introduced the trace event:
>
>    @@ -365,12 +368,16 @@
>                            if (rc)
>                                    kfree(key);
>     
>    +                       trace_mctp_key_acquire(key);
>    +
>                            /* we don't need to release key->lock on exit */
>                            key = NULL;
>   
> - which is from 4f9e1ba6de45. The unref() comes in later, but the
> initial uaf is caused by this change.
>
> So, I'd suggest this instead:
>
> Fixes: 4f9e1ba6de45 ("mctp: Add tracepoints for tag/key handling")
ok - see v2
>
> (this just means we need the fix for 5.16+, rather than 5.15+).
>
> Also, can you share how you're doing the clang static analysis there?
> I'll get that included in my checks too.

build clang, then use it

scan-build \
     --use-cc=clang \
     make CC=clang

There are a couple of configs that aren't happy with clang, these you 
can sed away with

sed -e 's/CONFIG_FRAME_WARN=2048/CONFIG_FRAME_WARN=0/; 
s/CONFIG_RETPOLINE=y/CONFIG_RETPOLINE=n/; 
s/CONFIG_READABLE_ASM=y/CONFIG_READABLE_ASM=n/; 
s/CONFIG_FORTIFY_SOURCE=y/CONFIG_FORTIFY_SOURCE=n/'

I am using clang 14

Tom

>
> Cheers,
>
>
> Jeremy
>