linux-kernel - Re: mm: NULL ptr deref in anon_vma_interval_tree

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] [day] [month] [year] [list]

Date:	Wed, 7 Nov 2012 17:57:35 +0800
From:	Bob Liu <lliubbo@...il.com>
To:	Michel Lespinasse <walken@...gle.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Sasha Levin <levinsasha928@...il.com>,
	Sasha Levin <sasha.levin@...cle.com>, hughd@...gle.com,
	linux-mm <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Dave Jones <davej@...hat.com>
Subject: Re: mm: NULL ptr deref in anon_vma_interval_tree_verify

Hi Michel,

On Wed, Nov 7, 2012 at 11:54 AM, Michel Lespinasse <walken@...gle.com> wrote:
> On Tue, Nov 6, 2012 at 12:24 AM, Michel Lespinasse <walken@...gle.com> wrote:
>> On Mon, Nov 5, 2012 at 5:41 AM, Michel Lespinasse <walken@...gle.com> wrote:
>>> On Sun, Nov 4, 2012 at 8:44 PM, Michel Lespinasse <walken@...gle.com> wrote:
>>>> On Sun, Nov 4, 2012 at 8:14 PM, Bob Liu <lliubbo@...il.com> wrote:
>>>>> Hmm, I attached a simple fix patch.
>>>>
>>>> Reviewed-by: Michel Lespinasse <walken@...gle.com>
>>>> (also ran some tests with it, but I could never reproduce the original
>>>> issue anyway).
>>>
>>> Wait a minute, this is actually wrong. You need to call
>>> vma_lock_anon_vma() / vma_unlock_anon_vma() to avoid the issue with
>>> vma->anon_vma == NULL.
>>>
>>> I'll fix it and integrate it into my next patch series, which I intend
>>> to send later today. (I am adding new code into validate_mm(), so that
>>> it's easier to have it in the same patch series to avoid merge
>>> conflicts)
>>
>> Hmmm, now I'm getting confused about anon_vma locking again :/
>>
>> As Hugh privately remarked to me, the same_vma linked list is supposed
>> to be protected by exclusive mmap_sem ownership, not by anon_vma lock.
>> So now looking at it a bit more, I'm not sure what race we're
>> preventing by taking the anon_vma lock in validate_mm() ???
>
> Looking at it a bit more:
>
> the same_vma linked list is *generally* protected by *exclusive*
> mmap_sem ownership. However, in expand_stack() we only have *shared*
> mmap_sem ownership, so that two concurrent expand_stack() calls
> (possibly on different vmas that have a different anon_vma lock) could
> race with each other. For this reason we do need the validate_mm()
> taking each vma's anon_vma lock (if any) before calling
> anon_vma_interval_tree_verify().
>

Sorry for the late response.
Actually my origin concern was:
avc was removed in some race place which caused the NULL pointer deref
in validate_mm().

But after looking it more, i didn't find out the race place.
I think avc only freed at free_pgtable() --> unlink_anon_vmas().

> While this justifies Bob's patch, this does not explain Sasha's
> reports - in both of them the backtrace did not involve
> expand_stack(), and there should be exclusive mmap_sem ownership, so
> I'm still unclear as to what could be causing Sasha's issue.
>
> Sasha, how reproduceable is this ?
>
> Also, would the following change print something when the issue triggers ?
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 619b280505fe..4c09e7ebcfa7 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -404,8 +404,13 @@ void validate_mm(struct mm_struct *mm)
>         while (vma) {
>                 struct anon_vma_chain *avc;
>                 vma_lock_anon_vma(vma);

And for our patch, i think vma_lock_anon_vma()/anon_vma_lock() is used
to protect
the same_anon_vma list.
It seems not suitable here.

> -               list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
> +               list_for_each_entry(avc, &vma->anon_vma_chain, same_vma) {
> +                       if (avc->vma != vma) {
> +                               printk("avc->vma %p vma %p\n", avc->vma, vma);
> +                               bug = 1;
> +                       }
>                         anon_vma_interval_tree_verify(avc);
> +               }
>                 vma_unlock_anon_vma(vma);
>                 highest_address = vma->vm_end;
>                 vma = vma->vm_next;
>
> --
> Michel "Walken" Lespinasse
> A program is never fully debugged until the last user dies.

-- 
Thanks,
--Bob
--
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/