From: Tom Rix <[email protected]>
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.
Fixes: 4a992bbd3650 ("mctp: Implement message fragmentation & reassembly")
Signed-off-by: Tom Rix <[email protected]>
---
net/mctp/route.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/mctp/route.c b/net/mctp/route.c
index 17e3482aa770..0c4c56e1bd6e 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -419,13 +419,14 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
* this function.
*/
rc = mctp_key_add(key, msk);
- if (rc)
+ if (rc) {
kfree(key);
+ } else {
+ trace_mctp_key_acquire(key);
- trace_mctp_key_acquire(key);
-
- /* we don't need to release key->lock on exit */
- mctp_key_unref(key);
+ /* we don't need to release key->lock on exit */
+ mctp_key_unref(key);
+ }
key = NULL;
} else {
--
2.26.3
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")
(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.
Cheers,
Jeremy
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
>
On Mon, Feb 14, 2022 at 6:16 PM Tom Rix <[email protected]> wrote:
>
>
> On 2/14/22 4:44 PM, Jeremy Kerr wrote:
> > Hi Tom,
> >
> > 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
I'm pretty sure we have a make target in Kbuild, too. It uses
clang-tidy as the driver, as clang-tidy can do BOTH the static
analyses AND clang-tidy checks.
$ make LLVM=1 all clang-analyzer
>
> 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
--
Thanks,
~Nick Desaulniers