linux-kernel - Re: [PATCH] ext4: mballoc: avoid garbage value from err

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6ffea4c-52e5-b13d-ae28-a3751995364c@nfschina.com>
Date:   Tue, 25 Jul 2023 12:21:39 +0800
From:   Su Hui <suhui@...china.com>
To:     Theodore Ts'o <tytso@....edu>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     adilger.kernel@...ger.ca, nathan@...nel.org, trix@...hat.com,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ext4: mballoc: avoid garbage value from err


On 2023/7/25 04:57, Theodore Ts'o wrote:
> On Mon, Jul 24, 2023 at 10:19:02AM -0700, Nick Desaulniers wrote:
>>> err is uninitialized and will be judged when it enters the
>>> loop first time and the condition "!ext4_sb_block_valid()"
>>> is true. Although this can't make problems now, it's better
>>> to correct it.
>>>
>>> Signed-off-by: Su Hui <suhui@...china.com>
>> Hi Su,
>> Thanks for the patch!  I see what the warning is getting at;
>>
>> If `len <= 0` then `err` is never initialized, then is used at line
>> 4178 (that is UB).
>>
>> Would you mind sending a v2 with the commit message updated to reflect
>> the above points?  I'd be happy to sign-off on that.
> Fortunately, as near as I can tell, ext4_mb_mark_bb() should never be
> called with len <= 0.  It might be possible to trick ext4 via a
> corrupted file system --- I'd have to take a closer look at that, but
> fortunately, in the case where len <= 0, bitmap_bh will be NULL, so
> regardless of whether err is 0, or some garbage non-zero value,
> brelse(NULL) is a no-op.
> So while it's good to avoid the clang warning, but the fact that it
> might be possible for err to be a "garbage value" shouldn't be causing
> any problem.
Maybe it can make the code more robust and clearer. I will send v2 patch 
soon.
Thanks for your reply!

Su Hui

>
> Cheers,
>
> 						- Ted