crypto: af_alg - Fix incorrect boolean values in af_alg_ctx
Subsystems (2)
PATCH 1/1 crypto: af_alg - Fix incorrect boolean values in af_alg_ctx
Commit 1b34cbbf4f01 ("crypto: af_alg - Disallow concurrent writes in af_alg_sendmsg") changed some fields from bool to 1-bit bitfields. However, some assignments to these fields, specifically 'more' and 'merge', assign values greater than 1. These relied on C's implicit conversion to bool, such that zero becomes false and nonzero becomes true. With a 1-bit bitfield instead, mod 2 of the value is taken instead, resulting in 0 being assigned in some cases when 1 was intended. Fix this by restoring the bool type.
--- include/crypto/if_alg.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
include/crypto/if_alg.h +5 -5
@@ -150,15 +150,15 @@ struct af_alg_ctx {struct crypto_wait wait;size_t used;atomic_t rcvused; - u32 more:1, - merge:1, - enc:1, - write:1, - init:1; +bool more; +bool merge; +bool enc; +bool write; +bool init;unsigned int len;unsigned int inflight; };
base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
Signature · 1 line
2.51.0.536.g15c5d4f767-goog
include/crypto/if_alg.h struct af_alg_ctx { · line 159
Quoted 11 lines · Eric Biggers
include/crypto/if_alg.h +5
-5- u32 more:1, - merge:1, - enc:1, - write:1, - init:1; +bool more; +bool merge; +bool enc; +bool write; +bool init;
This actually packs horribly, since a 'bool' will take up a byte for each, so now those five bits take up 8 bytes of storage (because the five bytes will then cause the next field to have to be aligned too).
You could just keep the bitfield format, but change the 'u32' to 'bool' and get the best of both worlds, ie just do something like
- u32 more:1,
+ bool more:1,
and now you get the bit packing _and_ the automatic bool behavior.
Linus
Quoted 23 lines · Linus Torvalds
include/crypto/if_alg.h struct af_alg_ctx { · line 159
Quoted 11 lines · Eric Biggers
include/crypto/if_alg.h +5
-5- u32 more:1, - merge:1, - enc:1, - write:1, - init:1; +bool more; +bool merge; +bool enc; +bool write; +bool init;This actually packs horribly, since a 'bool' will take up a byte for each, so now those five bits take up 8 bytes of storage (because the five bytes will then cause the next field to have to be aligned too).
You could just keep the bitfield format, but change the 'u32' to 'bool' and get the best of both worlds, ie just do something like
(quoted diff) +1
-1- u32 more:1, + bool more:1,
Quoted 1 line
and now you get the bit packing _and_ the automatic bool behavior.
Sure, I'll send out v2 with your suggestion.
I do think the idea of trying to re-pack the structure as part of a bug fix is a bit misguided, though. It's what caused this additional bug in the first place, and it's not like it actually matters here. (AF_ALG is rarely used, and when it is, the sockets tend not to be kept open for very long. And the entire concept of AF_ALG is a mistake anyway.)
- Eric
Quoted 3 lines · Eric Biggers
I do think the idea of trying to re-pack the structure as part of a bug fix is a bit misguided, though.
Well, now it's done, so let's not change it even *more*, when a one-liner should fix it.
I do agree that clearly the original fix was clearly buggy, but unless it's reverted entirely I'd rather go for "minimal one-liner fix on top of buggy fix", particularly since the end result is then better...
Linus