| Message ID | 20201111021427.466349-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block/keyslot-manager: prevent crash when num_slots=1 | expand |
On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > + * buckets. This only makes a difference when there is only 1 keyslot. > + */ > + slot_hashtable_size = max(slot_hashtable_size, 2U); shouldn't this be a min()?
On Wed, Nov 11, 2020 at 09:23:05AM +0000, Christoph Hellwig wrote: > On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > > + * buckets. This only makes a difference when there is only 1 keyslot. > > + */ > > + slot_hashtable_size = max(slot_hashtable_size, 2U); > > shouldn't this be a min()? I think it should be max(), since we want whichever is larger between 2 and the original slot_hashtable_size :)
On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > If there is only one keyslot, then blk_ksm_init() computes > slot_hashtable_size=1 and log_slot_ht_size=0. This causes > blk_ksm_find_keyslot() to crash later because it uses > hash_ptr(key, log_slot_ht_size) to find the hash bucket containing the > key, and hash_ptr() doesn't support the bits == 0 case. > > Fix this by making the hash table always have at least 2 buckets. > > Tested by running: > > kvm-xfstests -c ext4 -g encrypt -m inlinecrypt \ > -o blk-crypto-fallback.num_keyslots=1 > > Fixes: 1b2628397058 ("block: Keyslot Manager for Inline Encryption") > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > block/keyslot-manager.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c > index 35abcb1ec051d..0a5b2772324ad 100644 > --- a/block/keyslot-manager.c > +++ b/block/keyslot-manager.c > @@ -103,6 +103,13 @@ int blk_ksm_init(struct blk_keyslot_manager *ksm, unsigned int num_slots) > spin_lock_init(&ksm->idle_slots_lock); > > slot_hashtable_size = roundup_pow_of_two(num_slots); > + > + /* > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > + * buckets. This only makes a difference when there is only 1 keyslot. > + */ > + slot_hashtable_size = max(slot_hashtable_size, 2U); > + > ksm->log_slot_ht_size = ilog2(slot_hashtable_size); > ksm->slot_hashtable = kvmalloc_array(slot_hashtable_size, > sizeof(ksm->slot_hashtable[0]), > > base-commit: f8394f232b1eab649ce2df5c5f15b0e528c92091 > -- > 2.29.2 > Looks good to me. Please feel free to add Reviewed-by: Satya Tangirala <satyat@google.com>
On Wed, Nov 11, 2020 at 09:45:38AM +0000, Satya Tangirala wrote: > On Wed, Nov 11, 2020 at 09:23:05AM +0000, Christoph Hellwig wrote: > > On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > > > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > > > + * buckets. This only makes a difference when there is only 1 keyslot. > > > + */ > > > + slot_hashtable_size = max(slot_hashtable_size, 2U); > > > > shouldn't this be a min()? > I think it should be max(), since we want whichever is larger between 2 > and the original slot_hashtable_size :) max() is correct. I could just open-code it, if that would make it clearer: /* * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 * buckets. This only makes a difference when there is only 1 keyslot. */ if (slot_hashtable_size < 2) slot_hashtable_size = 2;
On Wed, Nov 11, 2020 at 11:25:39AM -0800, Eric Biggers wrote: > On Wed, Nov 11, 2020 at 09:45:38AM +0000, Satya Tangirala wrote: > > On Wed, Nov 11, 2020 at 09:23:05AM +0000, Christoph Hellwig wrote: > > > On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > > > > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > > > > + * buckets. This only makes a difference when there is only 1 keyslot. > > > > + */ > > > > + slot_hashtable_size = max(slot_hashtable_size, 2U); > > > > > > shouldn't this be a min()? > > I think it should be max(), since we want whichever is larger between 2 > > and the original slot_hashtable_size :) > > max() is correct. I could just open-code it, if that would make it clearer: > > /* > * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > * buckets. This only makes a difference when there is only 1 keyslot. > */ > if (slot_hashtable_size < 2) > slot_hashtable_size = 2; I sent out v2 with the above. - Eric
On Wed, Nov 11, 2020 at 09:45:38AM +0000, Satya Tangirala wrote: > On Wed, Nov 11, 2020 at 09:23:05AM +0000, Christoph Hellwig wrote: > > On Tue, Nov 10, 2020 at 06:14:27PM -0800, Eric Biggers wrote: > > > + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 > > > + * buckets. This only makes a difference when there is only 1 keyslot. > > > + */ > > > + slot_hashtable_size = max(slot_hashtable_size, 2U); > > > > shouldn't this be a min()? > I think it should be max(), since we want whichever is larger between 2 > and the original slot_hashtable_size :) Yes, of course. Sorry for the noise.
diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c index 35abcb1ec051d..0a5b2772324ad 100644 --- a/block/keyslot-manager.c +++ b/block/keyslot-manager.c @@ -103,6 +103,13 @@ int blk_ksm_init(struct blk_keyslot_manager *ksm, unsigned int num_slots) spin_lock_init(&ksm->idle_slots_lock); slot_hashtable_size = roundup_pow_of_two(num_slots); + + /* + * hash_ptr() assumes bits != 0, so ensure the hash table has at least 2 + * buckets. This only makes a difference when there is only 1 keyslot. + */ + slot_hashtable_size = max(slot_hashtable_size, 2U); + ksm->log_slot_ht_size = ilog2(slot_hashtable_size); ksm->slot_hashtable = kvmalloc_array(slot_hashtable_size, sizeof(ksm->slot_hashtable[0]),