svn commit: r361568 - head/sys/powerpc/aim

[Mark Millard]

Justin Hibbits chmeeedalf at gmail.com wrote on
Thu May 28 02:41:06 UTC 2020 :

> On Thu, 28 May 2020 00:49:03 +0000 (UTC)
> Brandon Bergren <bdragon at FreeBSD.org> wrote:
> 
> > Author: bdragon
> > Date: Thu May 28 00:49:02 2020
> > New Revision: 361568
> > URL: https://svnweb.freebsd.org/changeset/base/361568
> > 
> > Log:
> >   [PowerPC] Fix radix crash when passing -1 from userspace
> >   
> >   Found by running libc tests with radix enabled.
> >   
> >   Detect unsigned integer wrapping with a postcondition.
> >   
> >   Note: Radix MMU is not enabled by default yet.
> >   
> >   Sponsored by:	Tag1 Consulting, Inc.
> > 
> > Modified:
> >   head/sys/powerpc/aim/mmu_radix.c
> > 
> > Modified: head/sys/powerpc/aim/mmu_radix.c
> > ==============================================================================
> > --- head/sys/powerpc/aim/mmu_radix.c	Wed May 27 23:20:35
> > 2020	(r361567) +++ head/sys/powerpc/aim/mmu_radix.c	Thu
> > May 28 00:49:02 2020	(r361568) @@ -6000,7 +6000,8 @@
> > mmu_radix_kremove(vm_offset_t va) int mmu_radix_map_user_ptr(pmap_t
> > pm, volatile const void *uaddr, void **kaddr, size_t ulen, size_t
> > *klen) {
> > -	if ((uintptr_t)uaddr + ulen >= VM_MAXUSER_ADDRESS)
> > +	if ((uintptr_t)uaddr + ulen >= VM_MAXUSER_ADDRESS ||
> > +	    (uintptr_t)uaddr + ulen < (uintptr_t)uaddr)
> >  		return (EFAULT);
> >  
> >  	*kaddr = (void *)(uintptr_t)uaddr;
> 
> Wouldn't
> 
>     if ((uintptr_t)uaddr >= VM_MAXUSER_ADDRESS ||
>         (uintptr_t)uaddr + ulen >= VM_MAXUSER_ADDRESS)
> 
> be more appropriate?

Using:

#define  VM_MAXUSER_ADDRESS32    0xfffff000

as an example for 32-bit AIM powerpc.

Let (uintptr_t)uaddr==0xffffe000u
Let             ulen==    0x3000u

Then (uintptr_t)uaddr+ulen == 0x1000u
(wrapped/truncated: "Detect unsigned integer wrapping")

So (right hand sides forced unsigned
    by left hand sides being so):

(uintptr_t)uaddr<VM_MAXUSER_ADDRESS32
&&
(uintptr_t)uaddr+ulen<VM_MAXUSER_ADDRESS32

(making your if skip the code it controls access to).


Another way to write a test that avoids the wrap
messing things up is:

(unitptr_t)ulen  >= VM_MAXUSER_ADDRESS
||
(uintptr_t)uaddr >= (uintptr_t)VM_MAXUSER_ADDRESS - ulen


(I've left equality handling as it was, despite, for
example, 0xffffe000u with length 0x2000u having a last
address of 0xffffefffu and 0xffffefffu < 0xfffff000u .
There may be reasons to disallow that for all I know.)

===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)