Re: [RFC PATCH] bcache: fix stack corruption by PRECEDING_KEY()

From: Coly Li <colyli@suse.de>
To: Pierre JUHEN <pierre.juhen@orange.fr>,
	Rolf Fokkens <rolf@rolffokkens.nl>
Cc: Nix <nix@esperi.org.uk>, linux-block@vger.kernel.org
Subject: Re: [RFC PATCH] bcache: fix stack corruption by PRECEDING_KEY()
Date: Sun, 9 Jun 2019 16:23:46 +0800	[thread overview]
Message-ID: <c8acd15e-41e5-8c39-2c71-db809f2e0cce@suse.de> (raw)
In-Reply-To: <bf1ccdb8-2a32-e69b-f52e-0b4fa45fe18f@orange.fr>

On 2019/6/9 1:56 下午, Pierre JUHEN wrote:
> Le 09/06/2019 à 02:59, Coly Li a écrit :
>> On 2019/6/9 2:50 上午, Rolf Fokkens wrote:
>>> On 6/8/19 12:22 PM, Coly Li wrote:
>>>> +static inline void preceding_key(struct bkey *k, struct bkey
>>>> *preceding_key_p)
>>>> +{
>>>> +    if (KEY_INODE(k) || KEY_OFFSET(k)) {
>>>> +        *preceding_key_p = KEY(KEY_INODE(k), KEY_OFFSET(k), 0);
>>>> +        if (!preceding_key_p->low)
>>>> +            preceding_key_p->high--;
>>>> +        preceding_key_p->low--;
>>>> +    } else {
>>>> +        preceding_key_p = NULL;
>>> If I'm correct, the line above has no net effect, it just changes a
>>> local variable (parameter) with no effect elsewhere. So the else part
>>> may be left out, or do you mean this?
>>>
>>> *preceding_key_p = ZERO_KEY;
>>>
>> Hi Rolf and Pierre,
>>
>> Setting preceding_key_p to NULL is for the following
>> bch_btree_iter_init(). See the call chains
>>
>> bch_btree_insert_key()->bch_btree_iter_init()->
>> __bch_btree_iter_init()->bch_bset_search()
>>
>> preceding_key_p is parameter 'search' in bch_bset_search().
>> If it is NULL, t->data->start returns directly; if it is not NULL,
>> __bch_bset_search() is called.
>>
>> Indeed *preceding_key_p = ZERO_KEY is unnecessary, just makes me
>> comfortable. The problem is PRECEDING_KEY() allocates an on-stack
>> variable, and this one is overlapped with stackframe of
>> bch_btree_iter_init(), and overwritten. Because this anonymous on-stack
>> variable is allocated inside PRECEDING_KEY(), not (and should not be)
>> protected by compiler.
>>
>> So I add the new local variable preceding_key (and make preceding_key_p
>> points to it) explicitly on stack frame of bch_btree_insert_key(), which
>> will never be overlapped with stackframe of bch_btree_iter_init().
>>
>> Thanks.
> 
> HI,
> 
> 
> so the right line should be :
> 
> *preceding_key_p = NULL;
> 
> because Rolf is right
> 
> preceding_key_p = NULL;
> 
> does change only the value of the calling parameter and exits, not the
> value of the preceding key in the stack.

Hmm, can you talk more specific to the code ? I don't catch what you
mean ....  Thanks.

Coly Li

-- 

Coly Li