* [PATCH] mctp: fix use after free
@ 2022-02-14 17:51 trix
2022-02-15 0:44 ` Jeremy Kerr
0 siblings, 1 reply; 4+ messages in thread
From: trix @ 2022-02-14 17:51 UTC (permalink / raw)
To: jk, matt, davem, kuba, nathan, ndesaulniers
Cc: netdev, linux-kernel, llvm, Tom Rix
From: Tom Rix <trix@redhat.com>
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 <trix@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mctp: fix use after free
2022-02-14 17:51 [PATCH] mctp: fix use after free trix
@ 2022-02-15 0:44 ` Jeremy Kerr
2022-02-15 2:16 ` Tom Rix
0 siblings, 1 reply; 4+ messages in thread
From: Jeremy Kerr @ 2022-02-15 0:44 UTC (permalink / raw)
To: trix, matt, davem, kuba, nathan, ndesaulniers; +Cc: netdev, linux-kernel, llvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mctp: fix use after free
2022-02-15 0:44 ` Jeremy Kerr
@ 2022-02-15 2:16 ` Tom Rix
2022-02-15 19:42 ` Nick Desaulniers
0 siblings, 1 reply; 4+ messages in thread
From: Tom Rix @ 2022-02-15 2:16 UTC (permalink / raw)
To: Jeremy Kerr, matt, davem, kuba, nathan, ndesaulniers
Cc: netdev, linux-kernel, llvm
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mctp: fix use after free
2022-02-15 2:16 ` Tom Rix
@ 2022-02-15 19:42 ` Nick Desaulniers
0 siblings, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2022-02-15 19:42 UTC (permalink / raw)
To: Tom Rix, Jeremy Kerr
Cc: matt, davem, kuba, nathan, netdev, linux-kernel, llvm
On Mon, Feb 14, 2022 at 6:16 PM Tom Rix <trix@redhat.com> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-15 19:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-14 17:51 [PATCH] mctp: fix use after free trix
2022-02-15 0:44 ` Jeremy Kerr
2022-02-15 2:16 ` Tom Rix
2022-02-15 19:42 ` Nick Desaulniers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).