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: <84c50299-5b5b-867e-1e96-2d3a0c6ade2a@gmail.com>
Date:   Mon, 10 Apr 2023 20:58:21 +0800
From:   Peng Zhang <perlyzhang@...il.com>
To:     "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Peng Zhang <zhangpeng.00@...edance.com>,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, maple-tree@...ts.infradead.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 2/2] maple_tree: Fix a potential memory leak, OOB access,
 or other unpredictable bug


在 2023/4/10 20:43, Liam R. Howlett 写道:
> * Peng Zhang <zhangpeng.00@...edance.com> [230407 00:10]:
>> In mas_alloc_nodes(), there is such a piece of code:
>> while (requested) {
>> 	...
>> 	node->node_count = 0;
>> 	...
>> }
> You don't need to quote code in your commit message since it is
> available in the change log or in the file itself.
Ok, I will change it in the next version.
>
>> "node->node_count = 0" means to initialize the node_count field of the
>> new node, but the node may not be a new node. It may be a node that
>> existed before and node_count has a value, setting it to 0 will cause a
>> memory leak. At this time, mas->alloc->total will be greater than the
>> actual number of nodes in the linked list, which may cause many other
>> errors. For example, out-of-bounds access in mas_pop_node(), and
>> mas_pop_node() may return addresses that should not be used.
>> Fix it by initializing node_count only for new nodes.
>>
>> Fixes: 54a611b60590 ("Maple Tree: add new data structure")
>> Signed-off-by: Peng Zhang <zhangpeng.00@...edance.com>
>> Cc: <stable@...r.kernel.org>
>> ---
>>   lib/maple_tree.c | 16 ++++------------
>>   1 file changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
>> index 65fd861b30e1..9e25b3215803 100644
>> --- a/lib/maple_tree.c
>> +++ b/lib/maple_tree.c
>> @@ -1249,26 +1249,18 @@ static inline void mas_alloc_nodes(struct ma_state *mas, gfp_t gfp)
>>   	node = mas->alloc;
>>   	node->request_count = 0;
>>   	while (requested) {
>> -		max_req = MAPLE_ALLOC_SLOTS;
>> -		if (node->node_count) {
>> -			unsigned int offset = node->node_count;
>> -
>> -			slots = (void **)&node->slot[offset];
>> -			max_req -= offset;
>> -		} else {
>> -			slots = (void **)&node->slot;
>> -		}
>> -
>> +		max_req = MAPLE_ALLOC_SLOTS - node->node_count;
>> +		slots = (void **)&node->slot[node->node_count];
> Thanks, this is much cleaner.
>
>>   		max_req = min(requested, max_req);
>>   		count = mt_alloc_bulk(gfp, max_req, slots);
>>   		if (!count)
>>   			goto nomem_bulk;
>>   
>> +		if (node->node_count == 0)
>> +			node->slot[0]->node_count = 0;
>>   		node->node_count += count;
>>   		allocated += count;
>>   		node = node->slot[0];
>> -		node->node_count = 0;
>> -		node->request_count = 0;
> Why are we not clearing request_count anymore?
Because the node pointed to by the variable "node"
must not be the head node of the linked list at
this time, we only need to maintain the information
of the head node.
>
>>   		requested -= count;
>>   	}
>>   	mas->alloc->total = allocated;
>> -- 
>> 2.20.1
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ