On 1/6/21 9:40 AM, David Howells wrote: > David Howells <dhowe...@redhat.com> 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 <dhowe...@redhat.com> > 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 <t...@redhat.com> > Signed-off-by: David Howells <dhowe...@redhat.com> > > 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, ==, >