SPARC version of arch_validate_prot() looks broken (UAF read)

SPARC version of arch_validate_prot() looks broken (UAF read)

[Jann Horn]

From what I can tell from looking at the code:

SPARC's arch_validate_prot() looks up the VMA and peeks at it; that's
not permitted though. do_mprotect_pkey() calls arch_validate_prot()
before taking the mmap lock, so we can hit use-after-free reads if
someone concurrently deletes a VMA we're looking at.

Additionally, arch_validate_prot() currently only accepts the start
address as a parameter, but the SPARC code probably should be checking
the entire given range, which might consist of multiple VMAs?

I'm not sure what the best fix is here; it kinda seems like what SPARC
really wants is a separate hook that is called from inside the loop in
do_mprotect_pkey() that iterates over the VMAs? So maybe commit
9035cf9a97e4 ("mm: Add address parameter to arch_validate_prot()")
should be reverted, and a separate hook should be created?

(Luckily the ordering of the vmacache operations works out such that
AFAICS, despite calling find_vma() without holding the mmap_sem, we
can never end up establishing a vmacache entry with a dangling pointer
that might be considered valid on a subsequent call. So this should be
limited to a rather boring UAF data read, and not be exploitable for a
UAF write or UAF function pointer read.)

Re: SPARC version of arch_validate_prot() looks broken (UAF read)

[Khalid Aziz]

On 9/28/20 6:14 AM, Jann Horn wrote:
> From what I can tell from looking at the code:
> 
> SPARC's arch_validate_prot() looks up the VMA and peeks at it; that's
> not permitted though. do_mprotect_pkey() calls arch_validate_prot()
> before taking the mmap lock, so we can hit use-after-free reads if
> someone concurrently deletes a VMA we're looking at.

That makes sense. It will be a good idea to encapsulate vma access
inside sparc_validate_prot() between mmap_read_lock() and
mmap_read_unlock().

> 
> Additionally, arch_validate_prot() currently only accepts the start
> address as a parameter, but the SPARC code probably should be checking
> the entire given range, which might consist of multiple VMAs?
> 
> I'm not sure what the best fix is here; it kinda seems like what SPARC
> really wants is a separate hook that is called from inside the loop in
> do_mprotect_pkey() that iterates over the VMAs? So maybe commit
> 9035cf9a97e4 ("mm: Add address parameter to arch_validate_prot()")
> should be reverted, and a separate hook should be created?
> 
> (Luckily the ordering of the vmacache operations works out suIch that
> AFAICS, despite calling find_vma() without holding the mmap_sem, we
> can never end up establishing a vmacache entry with a dangling pointer
> that might be considered valid on a subsequent call. So this should be
> limited to a rather boring UAF data read, and not be exploitable for a
> UAF write or UAF function pointer read.)
> 

I think arch_validate_prot() is still the right hook to validate the
protection bits. sparc_validate_prot() can iterate over VMAs with read
lock. This will, of course, require range as well to be passed to
arch_validate_prot().

Thanks,
Khalid

Re: SPARC version of arch_validate_prot() looks broken (UAF read)

[Jann Horn]

On Tue, Sep 29, 2020 at 7:30 PM Khalid Aziz <khalid.aziz@oracle.com> wrote:
> On 9/28/20 6:14 AM, Jann Horn wrote:
> > From what I can tell from looking at the code:
> >
> > SPARC's arch_validate_prot() looks up the VMA and peeks at it; that's
> > not permitted though. do_mprotect_pkey() calls arch_validate_prot()
> > before taking the mmap lock, so we can hit use-after-free reads if
> > someone concurrently deletes a VMA we're looking at.
>
> That makes sense. It will be a good idea to encapsulate vma access
> inside sparc_validate_prot() between mmap_read_lock() and
> mmap_read_unlock().
>
> >
> > Additionally, arch_validate_prot() currently only accepts the start
> > address as a parameter, but the SPARC code probably should be checking
> > the entire given range, which might consist of multiple VMAs?
> >
> > I'm not sure what the best fix is here; it kinda seems like what SPARC
> > really wants is a separate hook that is called from inside the loop in
> > do_mprotect_pkey() that iterates over the VMAs? So maybe commit
> > 9035cf9a97e4 ("mm: Add address parameter to arch_validate_prot()")
> > should be reverted, and a separate hook should be created?
> >
> > (Luckily the ordering of the vmacache operations works out suIch that
> > AFAICS, despite calling find_vma() without holding the mmap_sem, we
> > can never end up establishing a vmacache entry with a dangling pointer
> > that might be considered valid on a subsequent call. So this should be
> > limited to a rather boring UAF data read, and not be exploitable for a
> > UAF write or UAF function pointer read.)
> >
>
> I think arch_validate_prot() is still the right hook to validate the
> protection bits. sparc_validate_prot() can iterate over VMAs with read
> lock. This will, of course, require range as well to be passed to
> arch_validate_prot().

In that case, do you want to implement this?
When I try to figure out how to implement this based on your
suggestion, it ends up a bit ugly because either mprotect has to do
some extra checks before calling the hook or the hook has to deal with
potentially (partly) unmapped userspace ranges in the supplied range
and then figure out what to do about those. (And for extra fun, it
also has to be safe against concurrent find_extend_vma() but should
probably also not change what happens when the first half of the given
address range is mapped and the second half is not mapped? Or does the
latter not matter?)
It'd also be annoying for me to test since I don't have any setup for
testing SPARC stuff (let alone SPARC ADI).

Re: SPARC version of arch_validate_prot() looks broken (UAF read)

[Christoph Hellwig]

On Wed, Oct 07, 2020 at 02:45:39AM +0200, Jann Horn wrote:
> > I think arch_validate_prot() is still the right hook to validate the
> > protection bits. sparc_validate_prot() can iterate over VMAs with read
> > lock. This will, of course, require range as well to be passed to
> > arch_validate_prot().
> 
> In that case, do you want to implement this?

Any reason to not just call arch_validate_prot after taking the mmap
lock?

Re: SPARC version of arch_validate_prot() looks broken (UAF read)

[Jann Horn]

On Wed, Oct 7, 2020 at 8:17 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Oct 07, 2020 at 02:45:39AM +0200, Jann Horn wrote:
> > > I think arch_validate_prot() is still the right hook to validate the
> > > protection bits. sparc_validate_prot() can iterate over VMAs with read
> > > lock. This will, of course, require range as well to be passed to
> > > arch_validate_prot().
> >
> > In that case, do you want to implement this?
>
> Any reason to not just call arch_validate_prot after taking the mmap
> lock?

Then the next easy steps are:

 - change the signature of arch_validate_prot() to also accept a
length or end parameter (because a start address without an end
address is completely useless)
 - add a loop over the VMAs in that range in SPARC's arch_validate_prot()

And then comes the annoying part: figure out what to do in that loop
if the range is not fully covered by VMAs. To fully avoid changing the
normal mprotect() ABI, you'd have to accept cases where parts of the
range are unmapped - but hopefully nobody relies on that particular
weirdness, so maybe you can just throw an error in that case? Even so,
the normal error code for that is -ENOMEM, but arch_validate_prot()
has a boolean return, so for that, you'd have to change the return
type, too. I guess the cleanest approach might be to just validate up
to the first gap and then return "everything's good" and rely on
mprotect() bailing out on its own?

Ah - at first I thought that you'd also have to deal with concurrent
stack VMA expansion from find_expand_vma() (which we really should get
rid of somehow sometime), but luckily that still at least holds the
mmap lock in read mode, and here we hold it in write mode, so we don't
have to worry about that. So I guess that'd be the way to go for this
approach?

Alright, I guess I'll send patches after all, hopefully after at least
compile-testing them...