mctp: fix use after free

Commit Message

Tom Rix Feb. 14, 2022, 5:51 p.m. UTC

From: Tom Rix <[e-mail]>

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 <[e-mail]>
---
 net/mctp/route.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Jeremy Kerr Feb. 15, 2022, 12:44 a.m. UTC | #1

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

Tom Rix Feb. 15, 2022, 2:16 a.m. UTC | #2

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
>

Nick Desaulniers Feb. 15, 2022, 7:42 p.m. UTC | #3

On Mon, Feb 14, 2022 at 6:16 PM Tom Rix <[e-mail]> 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

Patch

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 {