Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1570feed-489c-82f7-8d6b-9f53e9ebb87e@gmail.com>
Date:   Sat, 6 May 2023 09:48:19 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.1
Subject: Re: [PATCH] maple_tree: Fix potential out-of-bounds access in
 mas_wr_end_piv()
To:     "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, maple-tree@lists.infradead.org,
        Peng Zhang <zhangpeng.00@bytedance.com>
References: <20230504031422.47506-1-zhangpeng.00@bytedance.com>
 <20230506011447.2e47mf5kwwo4yz4r@revolver>
From:   Peng Zhang <perlyzhang@gmail.com>
In-Reply-To: <20230506011447.2e47mf5kwwo4yz4r@revolver>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/5/6 09:14, Liam R. Howlett 写道:
> * Peng Zhang <zhangpeng.00@bytedance.com> [230503 23:14]:
>> Access to the pivots array may be out of bounds. Fix it by changing the
>> code to ensure that the index of the pivots does not go out of bounds.
>> It is difficult to assess user-visible impact.
> 
> This is indeed an issue.  There isn't any user-visible impact for
> current node types, since the overflow will access the slots and be
> corrected in the next if clause, but it's certainly better to fix this.
> 
> The commit message is also not as descriptive as necessary, perhaps
> something like:
> 
> Check the write offset end bounds before using it as the offset into the
> pivot array.  This avoids a possible out-of-bounds access on the pivot
> array if the write extends to the last slot in the node, in which case
> the node maximum should be used as the end pivot.
> 
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> 
>>
>> Fixes: 54a611b60590 ("Maple Tree: add new data structure")
> 
> Cc stable ?
I don't know if it should be cc stable since Andrew says it
always needs to describe user-visible impact.
> 
>> Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com>
>> ---
>>   lib/maple_tree.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
>> index 110a36479dced..5a49327444d76 100644
>> --- a/lib/maple_tree.c
>> +++ b/lib/maple_tree.c
>> @@ -4263,11 +4263,13 @@ static inline bool mas_wr_slot_store(struct ma_wr_state *wr_mas)
>>   
>>   static inline void mas_wr_end_piv(struct ma_wr_state *wr_mas)
>>   {
>> -	while ((wr_mas->mas->last > wr_mas->end_piv) &&
>> -	       (wr_mas->offset_end < wr_mas->node_end))
>> -		wr_mas->end_piv = wr_mas->pivots[++wr_mas->offset_end];
>> +	while ((wr_mas->offset_end < wr_mas->node_end) &&
>> +	       (wr_mas->mas->last > wr_mas->pivots[wr_mas->offset_end]))
>> +		wr_mas->offset_end++;
>>   
>> -	if (wr_mas->mas->last > wr_mas->end_piv)
>> +	if (wr_mas->offset_end < wr_mas->node_end)
>> +		wr_mas->end_piv = wr_mas->pivots[wr_mas->offset_end];
>> +	else
>>   		wr_mas->end_piv = wr_mas->mas->max;
>>   }
>>   
>> @@ -4424,7 +4426,6 @@ static inline void *mas_wr_store_entry(struct ma_wr_state *wr_mas)
>>   	}
>>   
>>   	/* At this point, we are at the leaf node that needs to be altered. */
>> -	wr_mas->end_piv = wr_mas->r_max;
>>   	mas_wr_end_piv(wr_mas);
>>   
>>   	if (!wr_mas->entry)
>> -- 
>> 2.20.1
>>