linux-kernel - Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to vma

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]

Date:   Wed, 17 May 2023 18:51:42 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Lorenzo Stoakes <lstoakes@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to
 vma_merge()

* Peter Xu <peterx@...hat.com> [230517 09:50]:

...
> > > > 
> > > > I don't think this is safe.  You are telling vma_merge() something that
> > > > is not true and will result in can_vma_merge_before() passing.  I mean,
> > > > sure it will become true after you split (unless you can't?), but I
> > > > don't know if you can just merge a VMA that doesn't pass
> > > > can_vma_merge_before(), even for a short period?
> > > 
> > > I must admit I'm not really that handy yet to vma codes, so I could miss
> > > something obvious.
> > > 
> > > My reasoning comes from two parts that this pgoff looks all fine:
> > > 
> > > 1) It's documented in vma_merge() in that:
> > > 
> > >  * Given a mapping request (addr,end,vm_flags,file,pgoff,anon_name),
> > >  * figure out ...
> > > 
> > >   So fundamentally this pgoff is part of the mapping request paired with
> > >   all the rest of the information.  AFAICT it means it must match with what
> > >   "addr" is describing in VA address space.  That's why I think offseting
> > >   it makes sense here.
> > > 
> > >   It also matches my understanding in vma_merge() code on how the pgoff is
> > >   used.
> > > 
> > > 2) Uffd is nothing special in this regard, namely:
> > > 
> > >    mbind_range():
> > > 
> > > 	pgoff = vma->vm_pgoff + ((vmstart - vma->vm_start) >> PAGE_SHIFT);
> > > 	merged = vma_merge(vmi, vma->vm_mm, *prev, vmstart, vmend, vma->vm_flags,
> > > 			 vma->anon_vma, vma->vm_file, pgoff, new_pol,
> > > 			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > > 
> > >    mlock_fixup():
> > >    
> > > 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > > 	*prev = vma_merge(vmi, mm, *prev, start, end, newflags,
> > > 			vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > > 			vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > > 
> > >    mprotect_fixup():
> > > 
> > > 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > > 	*pprev = vma_merge(vmi, mm, *pprev, start, end, newflags,
> > > 			   vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > > 			   vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > > 
> > > I had a feeling that it's just something overlooked in the initial proposal
> > > of uffd, but maybe I missed something important?
> > 
> > I think you are correct.  It's worth noting that all of these skip
> > splitting if merging succeeds.
> 
> Yes, IIUC that's what we want because vma_merge() just handles everything
> there (including split, or say, vma range adjustments) if any !NULL
> returned.

I don't get your use of split here. __vma_adjust() used to be used by
split, but it never split a VMA.  vma_merge() is not used by split at
all.

> 
> > 
> > We know it won't match case 1-4 (we have a current vma).  We then pass
> > in vma_end = min(end, vma->vm_end);
> 
> Case 4 seems still possible and should be the case that mentioned in the
> patch 2, iiuc.  But yes I think vma_end calculation is needed, afaik it is
> to cover the last iteration, where that's the only place possible that we
> may operate on "end" (where < vma->vm_end) rather than "vma->vm_end".  It
> actually pairs with the initial "start" adjustment to me.
> 
> > 
> > vma_lookup() will only be called if end == vma->vm_end, so next will not
> > be set (and found) unless it is adjacent to the current vma and the vma
> > in question does not need to be split anyways.
> > 
> > I also see that we use pgoff+pglen in the check, which avoids my concern
> > above.
> 
> Right.
> 
> It seems so far all concerns are more or less ruled out.  I'll prepare a
> formal patchset, we can continue the discussion there.
> 
> Thanks,
> 
> -- 
> Peter Xu
>