rxrpc: fix handling of an unsupported token type in rxrpc_read()

Commit Message

Tom Rix Dec. 29, 2020, 5:39 p.m. UTC

From: Tom Rix <[e-mail]>

clang static analysis reports this problem

net/rxrpc/key.c:657:11: warning: Assigned value is garbage or undefined
                toksize = toksizes[tok++];
                        ^ ~~~~~~~~~~~~~~~

rxrpc_read() contains two loops.  The first loop calculates the token
sizes and stores the results in toksizes[] and the second one uses the
array.  When there is an error in identifying the token in the first
loop, the token is skipped, no change is made to the toksizes[] array.
When the same error happens in the second loop, the token is not
skipped.  This will cause the toksizes[] array to be out of step and
will overrun past the calculated sizes.

Change the error handling in the second loop to be consistent with
the first.  Simplify the error handling to an if check.

Fixes: 9a059cd5ca7d ("rxrpc: Downgrade the BUG() for unsupported token type in rxrpc_read()")
Signed-off-by: Tom Rix <[e-mail]>
---
 net/rxrpc/key.c | 48 ++++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

Comments

David Howells Jan. 4, 2021, 12:50 p.m. UTC | #1

[e-mail] wrote:

> -		switch (token->security_index) {
> -		case RXRPC_SECURITY_RXKAD:
> ...
> -		switch (token->security_index) {
> -		case RXRPC_SECURITY_RXKAD:

These switches need to be kept.  There's another security type on the way.
See:

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rxgk

for example.  I'll have a look later.

David

Tom Rix Jan. 4, 2021, 3:04 p.m. UTC | #2

On 1/4/21 4:50 AM, David Howells wrote:
> [e-mail] wrote:
>
>> -		switch (token->security_index) {
>> -		case RXRPC_SECURITY_RXKAD:
>> ...
>> -		switch (token->security_index) {
>> -		case RXRPC_SECURITY_RXKAD:
> These switches need to be kept.  There's another security type on the way.
> See:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rxgk
>
> for example.  I'll have a look later.

Yes, looks like more stuff is coming.

Thanks!

Tom

> David
>

David Howells Jan. 6, 2021, 4:57 p.m. UTC | #3

How about this?

David
---
commit 5d370a9db65a6fae82f09a009430ae40c564b0ef
Author: David Howells <[e-mail]>
Date:   Wed Jan 6 16:21:40 2021 +0000

    rxrpc: Fix handling of an unsupported token type in rxrpc_read()
    
    Clang static analysis reports the following:
    
    net/rxrpc/key.c:657:11: warning: Assigned value is garbage or undefined
                    toksize = toksizes[tok++];
                            ^ ~~~~~~~~~~~~~~~
    
    rxrpc_read() contains two consecutive loops.  The first loop calculates the
    token sizes and stores the results in toksizes[] and the second one uses
    the array.  When there is an error in identifying the token in the first
    loop, the token is skipped, no change is made to the toksizes[] array.
    When the same error happens in the second loop, the token is not skipped.
    This will cause the toksizes[] array to be out of step and will overrun
    past the calculated sizes.
    
    Fix the second loop so that it doesn't encode the size and type of an
    unsupported token, but rather just ignore it as does the first loop.
    
    Fixes: 9a059cd5ca7d ("rxrpc: Downgrade the BUG() for unsupported token type in rxrpc_read()")
    Reported-by: Tom Rix <[e-mail]>
    Signed-off-by: David Howells <[e-mail]>

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 9631aa8543b5..c8e298c8d314 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -655,12 +655,12 @@ static long rxrpc_read(const struct key *key,
 	tok = 0;
 	for (token = key->payload.data[0]; token; token = token->next) {
 		toksize = toksizes[tok++];
-		ENCODE(toksize);
 		oldxdr = xdr;
-		ENCODE(token->security_index);
 
 		switch (token->security_index) {
 		case RXRPC_SECURITY_RXKAD:
+			ENCODE(toksize);
+			ENCODE(token->security_index);
 			ENCODE(token->kad->vice_id);
 			ENCODE(token->kad->kvno);
 			ENCODE_BYTES(8, token->kad->session_key);

David Howells Jan. 6, 2021, 5:40 p.m. UTC | #4

David Howells <[e-mail]> wrote:

> How about this?
> ...
>     Fix the second loop so that it doesn't encode the size and type of an
>     unsupported token, but rather just ignore it as does the first loop.

Actually, a better way is probably just to error out in this case.  This
should only happen if a new token type is incompletely implemented.

David
---
commit e68ef16f59aa57564761b21e5ecb2ebbd72d1c57
Author: David Howells <[e-mail]>
Date:   Wed Jan 6 16:21:40 2021 +0000

    rxrpc: Fix handling of an unsupported token type in rxrpc_read()
    
    Clang static analysis reports the following:
    
    net/rxrpc/key.c:657:11: warning: Assigned value is garbage or undefined
                    toksize = toksizes[tok++];
                            ^ ~~~~~~~~~~~~~~~
    
    rxrpc_read() contains two consecutive loops.  The first loop calculates the
    token sizes and stores the results in toksizes[] and the second one uses
    the array.  When there is an error in identifying the token in the first
    loop, the token is skipped, no change is made to the toksizes[] array.
    When the same error happens in the second loop, the token is not skipped.
    This will cause the toksizes[] array to be out of step and will overrun
    past the calculated sizes.
    
    Fix this by making both loops log a message and return an error in this
    case.  This should only happen if a new token type is incompletely
    implemented, so it should normally be impossible to trigger this.
    
    Fixes: 9a059cd5ca7d ("rxrpc: Downgrade the BUG() for unsupported token type in rxrpc_read()")
    Reported-by: Tom Rix <[e-mail]>
    Signed-off-by: David Howells <[e-mail]>

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 9631aa8543b5..8d2073e0e3da 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -598,7 +598,7 @@ static long rxrpc_read(const struct key *key,
 		default: /* we have a ticket we can't encode */
 			pr_err("Unsupported key token type (%u)\n",
 			       token->security_index);
-			continue;
+			return -ENOPKG;
 		}
 
 		_debug("token[%u]: toksize=%u", ntoks, toksize);
@@ -674,7 +674,9 @@ static long rxrpc_read(const struct key *key,
 			break;
 
 		default:
-			break;
+			pr_err("Unsupported key token type (%u)\n",
+			       token->security_index);
+			return -ENOPKG;
 		}
 
 		ASSERTCMP((unsigned long)xdr - (unsigned long)oldxdr, ==,

Tom Rix Jan. 6, 2021, 7:34 p.m. UTC | #5

On 1/6/21 9:40 AM, David Howells wrote:
> David Howells <[e-mail]> wrote:
>
>> How about this?
>> ...
>>     Fix the second loop so that it doesn't encode the size and type of an
>>     unsupported token, but rather just ignore it as does the first loop.
> Actually, a better way is probably just to error out in this case.  This
> should only happen if a new token type is incompletely implemented.
>
> David
> ---
> commit e68ef16f59aa57564761b21e5ecb2ebbd72d1c57
> Author: David Howells <[e-mail]>
> Date:   Wed Jan 6 16:21:40 2021 +0000
>
>     rxrpc: Fix handling of an unsupported token type in rxrpc_read()
>     
>     Clang static analysis reports the following:
>     
>     net/rxrpc/key.c:657:11: warning: Assigned value is garbage or undefined
>                     toksize = toksizes[tok++];
>                             ^ ~~~~~~~~~~~~~~~
>     
>     rxrpc_read() contains two consecutive loops.  The first loop calculates the
>     token sizes and stores the results in toksizes[] and the second one uses
>     the array.  When there is an error in identifying the token in the first
>     loop, the token is skipped, no change is made to the toksizes[] array.
>     When the same error happens in the second loop, the token is not skipped.
>     This will cause the toksizes[] array to be out of step and will overrun
>     past the calculated sizes.
>     
>     Fix this by making both loops log a message and return an error in this
>     case.  This should only happen if a new token type is incompletely
>     implemented, so it should normally be impossible to trigger this.
>     
>     Fixes: 9a059cd5ca7d ("rxrpc: Downgrade the BUG() for unsupported token type in rxrpc_read()")
>     Reported-by: Tom Rix <[e-mail]>
>     Signed-off-by: David Howells <[e-mail]>
>
> diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
> index 9631aa8543b5..8d2073e0e3da 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c
> @@ -598,7 +598,7 @@ static long rxrpc_read(const struct key *key,
>  		default: /* we have a ticket we can't encode */
>  			pr_err("Unsupported key token type (%u)\n",
>  			       token->security_index);
> -			continue;
> +			return -ENOPKG;
>  		}

These two loops iterate over the same data, i believe returning here is all that is needed.

Tom

>  
>  		_debug("token[%u]: toksize=%u", ntoks, toksize);
> @@ -674,7 +674,9 @@ static long rxrpc_read(const struct key *key,
>  			break;
>  
>  		default:
> -			break;
> +			pr_err("Unsupported key token type (%u)\n",
> +			       token->security_index);
> +			return -ENOPKG;
>  		}
>  
>  		ASSERTCMP((unsigned long)xdr - (unsigned long)oldxdr, ==,
>

David Howells Jan. 6, 2021, 7:44 p.m. UTC | #6

Tom Rix <[e-mail]> wrote:

> These two loops iterate over the same data, i believe returning here is all
> that is needed.

But if the first loop is made to support a new type, but the second loop is
missed, it will then likely oops.  Besides, the compiler should optimise both
paths together.

David

Tom Rix Jan. 6, 2021, 8:38 p.m. UTC | #7

On 1/6/21 11:44 AM, David Howells wrote:
> Tom Rix <[e-mail]> wrote:
>
>> These two loops iterate over the same data, i believe returning here is all
>> that is needed.
> But if the first loop is made to support a new type, but the second loop is
> missed, it will then likely oops.  Besides, the compiler should optimise both
> paths together.

You are right, I was only considering the existing cases.

Tom

> David
>

David Howells Jan. 6, 2021, 9:09 p.m. UTC | #8

Tom Rix <[e-mail]> wrote:

> On 1/6/21 11:44 AM, David Howells wrote:
> > Tom Rix <[e-mail]> wrote:
> >
> >> These two loops iterate over the same data, i believe returning here is all
> >> that is needed.
> > But if the first loop is made to support a new type, but the second loop is
> > missed, it will then likely oops.  Besides, the compiler should optimise both
> > paths together.
> 
> You are right, I was only considering the existing cases.

Thanks.  Can I put that down as a Reviewed-by?

David

Tom Rix Jan. 6, 2021, 10:09 p.m. UTC | #9

On 1/6/21 1:09 PM, David Howells wrote:
> Tom Rix <[e-mail]> wrote:
>
>> On 1/6/21 11:44 AM, David Howells wrote:
>>> Tom Rix <[e-mail]> wrote:
>>>
>>>> These two loops iterate over the same data, i believe returning here is all
>>>> that is needed.
>>> But if the first loop is made to support a new type, but the second loop is
>>> missed, it will then likely oops.  Besides, the compiler should optimise both
>>> paths together.
>> You are right, I was only considering the existing cases.
> Thanks.  Can I put that down as a Reviewed-by?

Yes, please.

Reviewed-by: Tom Rix <[e-mail]>

>
> David
>

Patch

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 9631aa8543b5..eea877ee6ab3 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -587,20 +587,19 @@  static long rxrpc_read(const struct key *key,
 	for (token = key->payload.data[0]; token; token = token->next) {
 		toksize = 4;	/* sec index */
 
-		switch (token->security_index) {
-		case RXRPC_SECURITY_RXKAD:
-			toksize += 8 * 4;	/* viceid, kvno, key*2, begin,
-						 * end, primary, tktlen */
-			if (!token->no_leak_key)
-				toksize += RND(token->kad->ticket_len);
-			break;
-
-		default: /* we have a ticket we can't encode */
+		if (token->security_index != RXRPC_SECURITY_RXKAD) {
+			/* we have a ticket we can't encode */
 			pr_err("Unsupported key token type (%u)\n",
 			       token->security_index);
 			continue;
 		}
 
+		/* viceid, kvno, key*2, begin, end, primary, tktlen */
+		toksize += 8 * 4;
+
+		if (!token->no_leak_key)
+			toksize += RND(token->kad->ticket_len);
+
 		_debug("token[%u]: toksize=%u", ntoks, toksize);
 		ASSERTCMP(toksize, <=, AFSTOKEN_LENGTH_MAX);
 
@@ -654,28 +653,25 @@  static long rxrpc_read(const struct key *key,
 
 	tok = 0;
 	for (token = key->payload.data[0]; token; token = token->next) {
+		/* error reported above */
+		if (token->security_index != RXRPC_SECURITY_RXKAD)
+			continue;
+
 		toksize = toksizes[tok++];
 		ENCODE(toksize);
 		oldxdr = xdr;
 		ENCODE(token->security_index);
 
-		switch (token->security_index) {
-		case RXRPC_SECURITY_RXKAD:
-			ENCODE(token->kad->vice_id);
-			ENCODE(token->kad->kvno);
-			ENCODE_BYTES(8, token->kad->session_key);
-			ENCODE(token->kad->start);
-			ENCODE(token->kad->expiry);
-			ENCODE(token->kad->primary_flag);
-			if (token->no_leak_key)
-				ENCODE(0);
-			else
-				ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
-			break;
-
-		default:
-			break;
-		}
+		ENCODE(token->kad->vice_id);
+		ENCODE(token->kad->kvno);
+		ENCODE_BYTES(8, token->kad->session_key);
+		ENCODE(token->kad->start);
+		ENCODE(token->kad->expiry);
+		ENCODE(token->kad->primary_flag);
+		if (token->no_leak_key)
+			ENCODE(0);
+		else
+			ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
 
 		ASSERTCMP((unsigned long)xdr - (unsigned long)oldxdr, ==,
 			  toksize);