Re: [PATCH] mctp: fix use after free

[Tom Rix]

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