linux-kernel - Re: [PATCH] Fix use-after-free of q->root_blkg and q->root

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: <507F6FE2.3030303@ce.jp.nec.com>
Date:	Thu, 18 Oct 2012 11:56:34 +0900
From:	"Jun'ichi Nomura" <j-nomura@...jp.nec.com>
To:	Vivek Goyal <vgoyal@...hat.com>, Tejun Heo <tj@...nel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Fix use-after-free of q->root_blkg and q->root_rl.blkg

On 10/17/12 22:47, Vivek Goyal wrote:
> On Wed, Oct 17, 2012 at 09:02:22AM +0900, Jun'ichi Nomura wrote:
>> On 10/17/12 08:20, Tejun Heo wrote:
>>>>>> -	if (ent == &q->root_blkg->q_node)
>>>>>> +	if (q->root_blkg && ent == &q->root_blkg->q_node)
>>>>>
>>>>> Can we fix it little differently. Little earlier in the code, we check for
>>>>> if q->blkg_list is empty, then all the groups are gone, and there are
>>>>> no more request lists hence and return NULL.
>>>>>
>>>>> Current code:
>>>>>         if (rl == &q->root_rl) {
>>>>>                 ent = &q->blkg_list;
>>>>>
>>>>> Modified code:
>>>>>         if (rl == &q->root_rl) {
>>>>>                 ent = &q->blkg_list;
>>>>> 		/* There are no more block groups, hence no request lists */
>>>>> 		if (list_empty(ent))
>>>>> 			return NULL;
>>>>> 	}
>>>
>>> Do we need this at all?  q->root_blkg being NULL is completely fine
>>> there and the comparison would work as expected, no?
>>
>> Hmm?
>>
>> If list_empty(ent) and q->root_blkg == NULL,
>>
>>>         /* walk to the next list_head, skip root blkcg */
>>>         ent = ent->next;
>>
>> ent is &q->blkg_list again.
>>
>>>         if (ent == &q->root_blkg->q_node)
>>
>> So ent is not &q->root_blkg->q_node.
> 
> If q->root_blkg is NULL, will it not lead to NULL pointer dereference.
> (q->root_blkg->q_node).

It's not dereferenced.

>>>                 ent = ent->next;
>>>         if (ent == &q->blkg_list)
>>>                 return NULL;
>>
>> And we return NULL here.
>>
>> Ah, yes. You are correct.
>> We can do without the above hunk.
> 
> I would rather prefer to check for this boundary condition early and
> return instead of letting it fall through all these conditions and
> then figure out yes we have no next rl. IMO, code becomes easier to
> understand if nothing else. Otherwise one needs a step by step 
> explanation as above to show that case of q->root_blkg is covered.

I have same opinion as yours that it's good for readability.

-- 
Jun'ichi Nomura, NEC Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/