linux-kernel - Re: [PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <382408d3-21ed-4bb3-87a2-60ad61583726@gmail.com>
Date: Tue, 12 Nov 2024 16:08:37 +0100
From: Gianfranco Trad <gianf.trad@...il.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: linux-bcachefs@...r.kernel.org, linux-kernel@...r.kernel.org,
 skhan@...uxfoundation.org,
 syzbot+8689d10f1894eedf774d@...kaller.appspotmail.com
Subject: Re: [PATCH] bcachefs: zero-init move_bucket struct in
 bch2_copygc_get_buckets()

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@...kaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=8689d10f1894eedf774d
>> Tested-by: syzbot+8689d10f1894eedf774d@...kaller.appspotmail.com
>> Signed-off-by: Gianfranco Trad <gianf.trad@...il.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