On Apr 13, 2024, at 8:43 AM, Nam Cao <namcao@xxxxxxxxxxxxx> wrote:
>
> On 2024-04-12 Björn Töpel wrote:
>>
>> What I see in ext4_search_dir() is that search_buf is 0xfffff000, and at
>> some point the address wraps to zero, and boom. I doubt that 0xfffff000
>> is a sane address.
>
> I have zero knowledge about file system, but I think it's an integer
> overflow problem. The calculation of "dlimit" overflow and dlimit wraps
> around, this leads to wrong comparison later on.
>
> I guess that explains why your bisect and Conor's bisect results are
> strange: the bug has been here for quite some time, but it only appears
> when "dlimit" happens to overflow.
>
> It can be fixed by re-arrange the comparisons a bit. Can you give the
> below patch a try?
>
> Best regards,
> Nam
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 05b647e6bc19..71b88b33b676 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1532,15 +1532,13 @@ int ext4_search_dir(struct buffer_head *bh, char *search_buf, int buf_size,
> unsigned int offset, struct ext4_dir_entry_2 **res_dir)
> {
> struct ext4_dir_entry_2 * de;
> - char * dlimit;
> int de_len;
>
> de = (struct ext4_dir_entry_2 *)search_buf;
> - dlimit = search_buf + buf_size;
> - while ((char *) de < dlimit - EXT4_BASE_DIR_LEN) {
> + while ((char *) de - search_buf < buf_size - EXT4_BASE_DIR_LEN) {
> /* this code is executed quadratically often */
> /* do minimal checking `by hand' */
> - if (de->name + de->name_len <= dlimit &&
> + if (de->name + de->name_len - search_buf <= buf_size &&
> ext4_match(dir, fname, de)) {
> /* found a match - just to be sure, do
> * a full check */
This looks like a straight-forward mathematical substitution of "dlimit"
with "search_buf + buf_size" and rearranging of the terms to make the
while loop offset "zero based" rather than "address based" and would
avoid overflow if "search_buf" was within one 4kB block of overflow:
dlimit = search_buf + buf_size = 0xfffff000 + 0x1000 = 0x00000000
If (char *)de is signed then this loop would run for a long time.
It doesn't look like it would have any issues with *underflow* (since
"de == search_buf" at the start and is only incremented, and there is
no valid filesystem where "buf_size < EXT4_BASE_DIR_LEN".
As such, I think this change would likely address the issue.
As to whether the 0xfffff000 address itself is valid for riscv32 is
outside my realm, but given that RAM is cheap it doesn't seem unlikely
to have 4GB+ of RAM and want to use it all. The riscv32 might consider
reserving this page address from allocation to avoid similar issues in
other parts of the code, as is done with the NULL/0 page address.
If you submit this as a proper patch you could add my:
Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
