PING: [PATCH] Don't overflow when writing a key

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "richard" <richard at nod.at>
> An: "Jan-Benedict Glaw" <jbglaw at lug-owl.de>
> CC: "linux-mtd" <linux-mtd at lists.infradead.org>, "Artem Bityutskiy" <artem.bityutskiy at linux.intel.com>
> Gesendet: Donnerstag, 7. Oktober 2021 08:59:03
> Betreff: Re: PING: [PATCH] Don't overflow when writing a key

> ----- Ursprüngliche Mail -----
>> Von: "Jan-Benedict Glaw" <jbglaw at lug-owl.de>
>> An: "linux-mtd" <linux-mtd at lists.infradead.org>
>> CC: "Artem Bityutskiy" <artem.bityutskiy at linux.intel.com>, "richard"
>> <richard at nod.at>
>> Gesendet: Dienstag, 5. Oktober 2021 16:49:36
>> Betreff: PING: [PATCH] Don't overflow when writing a key
> 
>> Hi,
>> 
>> On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw at lug-owl.de> wrote:
>>> diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
>>> index 8142d9d6fe5d..40edcca7ba62 100644
>>> --- a/fs/ubifs/key.h
>>> +++ b/fs/ubifs/key.h
>>> @@ -436,7 +436,7 @@ static inline void key_write(const struct ubifs_info *c,
>>>  
>>>  	t->j32[0] = cpu_to_le32(from->u32[0]);
>>>  	t->j32[1] = cpu_to_le32(from->u32[1]);
>>> -	memset(to + 8, 0, UBIFS_MAX_KEY_LEN - 8);
>>> +	memset(to + 8, 0, UBIFS_SK_LEN - 8);
>>>  }
>>>  
>>>  /**
>> 
>> I wanted to give this little patch a ping since there wasn't a reply
> 
> In MTD's patchwork it is marked as "Under review", so it is not lost.
> 
>> until now and I think it might fix an overflow.
> 
> Your fix looks legit but since I'm traveling I need to give it a deeper
> thought when I'm back home.
> I'm a little puzzled why nobody noticed the stack corruption so far,
> key_write() has been doing since ever.

Typing that mail triggered already a deeper thought. :-D

key_write() takes a ubifs_key as source and writes to a buffer.
It assumes that the passed ubifs_key is in simple key format (being UBIFS_SK_LEN long)
and it assumes the buffer is UBIFS_MAX_KEY_LEN long.
So, the destination is *not* a union ubifs_key structure.

We have three callers of key_write(), all in journal.c

1. ubifs_jnl_write_data
key_write(c, key, &data->key);

data is struct ubifs_data_node where key is of size UBIFS_MAX_KEY_LEN.

2. ubifs_jnl_update

Same with struct ubifs_dent_node

3. ubifs_jnl_delete_xattr

Same again.

All three objects are slab allocated, did you check, is too few memory allocated?
In other words, how did you calculate the size of target buffer?

Thanks,
//richard