1. Patchew
  2. linux
  3. View series

[PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get_buckets()

Gianfranco Trad posted 1 patch 1 week, 5 days ago 
fs/bcachefs/movinggc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get_buckets()
Posted by Gianfranco Trad 1 week, 5 days ago 
zero-init move_bucket struct b fields in bch2_copygc_get_buckets() 
to mitigate later uninit-value-use KMSAN reported bug.

Reported-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8689d10f1894eedf774d
Tested-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
---
 fs/bcachefs/movinggc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
index d658be90f737..cdc456b03bec 100644
--- a/fs/bcachefs/movinggc.c
+++ b/fs/bcachefs/movinggc.c
@@ -171,7 +171,8 @@ static int bch2_copygc_get_buckets(struct moving_context *ctxt,
 				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
 				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
 				  0, k, ({
-		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
+		struct move_bucket b = { 0 };
+		b.k.bucket = u64_to_bucket(k.k->p.offset);
 		int ret2 = 0;
 
 		saw++;
-- 
2.43.0
Re: [PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get_buckets()
Posted by Kent Overstreet 1 week, 4 days ago 
On Mon, Nov 11, 2024 at 03:42:44PM +0100, Gianfranco Trad wrote:
> zero-init move_bucket struct b fields in bch2_copygc_get_buckets() 
> to mitigate later uninit-value-use KMSAN reported bug.
> 
> Reported-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=8689d10f1894eedf774d
> Tested-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
> ---
>  fs/bcachefs/movinggc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
> index d658be90f737..cdc456b03bec 100644
> --- a/fs/bcachefs/movinggc.c
> +++ b/fs/bcachefs/movinggc.c
> @@ -171,7 +171,8 @@ static int bch2_copygc_get_buckets(struct moving_context *ctxt,
>  				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
>  				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
>  				  0, k, ({
> -		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
> +		struct move_bucket b = { 0 };
> +		b.k.bucket = u64_to_bucket(k.k->p.offset);
>  		int ret2 = 0;

Providing any sort of initializer should cause the whole struct to be
initialized, are you and syzbot sure this is the right fix?
Re: [PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get_buckets()
Posted by Gianfranco Trad 1 week, 4 days ago 
On 11/11/24 21:09, Kent Overstreet wrote:
> On Mon, Nov 11, 2024 at 03:42:44PM +0100, Gianfranco Trad wrote:
>> zero-init move_bucket struct b fields in bch2_copygc_get_buckets()
>> to mitigate later uninit-value-use KMSAN reported bug.
>>
>> Reported-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=8689d10f1894eedf774d
>> Tested-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
>> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
>> ---
>>   fs/bcachefs/movinggc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
>> index d658be90f737..cdc456b03bec 100644
>> --- a/fs/bcachefs/movinggc.c
>> +++ b/fs/bcachefs/movinggc.c
>> @@ -171,7 +171,8 @@ static int bch2_copygc_get_buckets(struct moving_context *ctxt,
>>   				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
>>   				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
>>   				  0, k, ({
>> -		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
>> +		struct move_bucket b = { 0 };
>> +		b.k.bucket = u64_to_bucket(k.k->p.offset);
>>   		int ret2 = 0;
> 
> Providing any sort of initializer should cause the whole struct to be
> initialized, are you and syzbot sure this is the right fix?
You are right, there's no need to initialize the whole struct.
I'm still in the process of fully understanding what reproducer is 
trying to do.
So far with the additional findings, b.k seems not to be initialized 
prior usage in repro case, therefore memset to 0 only the b.k field 
seems enough:

diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
index d658be90f737..515b05d26d11 100644
--- a/fs/bcachefs/movinggc.c
+++ b/fs/bcachefs/movinggc.c
@@ -171,7 +171,9 @@ static int bch2_copygc_get_buckets(struct 
moving_context *ctxt,
  				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
  				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
  				  0, k, ({
-		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
+		struct move_bucket b;
+		memset(&b.k, 0, sizeof(b.k));
+		b.k.bucket = u64_to_bucket(k.k->p.offset);
  		int ret2 = 0;

  		saw++;

The above patch was already tested-by syzbot[1].

Let me know if the patch looks good enough or if I should work on it more.

Thanks for your time,

[1] https://syzkaller.appspot.com/x/log.txt?x=1733b8c0580000

--Gian

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.