[prev in list] [next in list] [prev in thread] [next in thread] 

List:       oss-security
Subject:    [oss-security] kernel: uio: CVE-2013-6763 [was: Re: [oss-security] some unstracked linux kernel secu
From:       Petr Matousek <pmatouse () redhat ! com>
Date:       2013-11-26 12:18:39
Message-ID: 20131126121838.GG9509 () dhcp-25-225 ! brq ! redhat ! com
[Download RAW message or body]

Adding Greg as he's also UIO maintainer (at least according to
MAINTAINERS).

On Thu, Nov 14, 2013 at 05:52:12PM +0100, Petr Matousek wrote:
> On Thu, Nov 14, 2013 at 04:25:39PM +0300, Dan Carpenter wrote:
> > On Thu, Nov 14, 2013 at 11:33:10AM +0100, Petr Matousek wrote:
> > > On Tue, Nov 12, 2013 at 11:10:32AM +0100, Petr Matousek wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Nov 03, 2013 at 05:32:52PM +0100, Nico Golde wrote:
> > > > > drivers/uio/uio.c: mapping of physical memory to user space without proper size check
> > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7314e613d5ff
> > > > 
> > > > there is a size check in uio_mmap() (the only caller of uio_mmap_physical()):
> > > > 
> > > >         requested_pages = vma_pages(vma);
> > > >         actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK)
> > > >                         + idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
> > > >         if (requested_pages > actual_pages)
> > > >                 return -EINVAL;
> > > > 
> > > > why it wasn't sufficient?
> > > 
> > > Apparently there was a CVE split [1] and this is now CVE-2013-6763.
> > > 
> > >   http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-6763
> > > 
> > > I still think this is a non-issue based on the above mentioned size
> > > check. Can I please get second opinion from someone more knowledgeable
> > > on this?
> > > 
> > >
> > 
> > Added Hans to the CC list since he's the maintainer.  Petr is asking if
> > the size checks in uio_mmap() and uio_mmap_physical() are duplicative.
> > 
> > > Isn't the size check redundant because of 
> > > 
> > >         requested_pages = vma_pages(vma);
> > >         actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK)
> > >                         + idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
> > >         if (requested_pages > actual_pages)
> > >                 return -EINVAL;
> > 
> > That check is worrying requested_pages is rounded down to the nearest
> > page
> 
> Is there any rounding down happening? I would expect both vma->vm_start
> and vma->vm_end to be page aligned.
> 
> > but actual_pages is rounded up. I don't understand why we are
> > adding "(mem[mi]addr % PAGE_SIZE)" to the pre rounded up actual_pages.
> 
> Imagine addr and size are not page aligned and
> ((addr & ~PAGE_MASK) + (size & ~PAGE_MASK)) > PAGE_SIZE.
> We need to round up two pages instead of one in that case.
> 
> > So, yeah, it seems like we do check the size twice now except the first
> > time we do it wrong.
> 
> With unaligned addr and/or size we can end up with mapping memory 
> not belonging to the UIO_MEM_PHYS registered region, but that is something
> you expect when using this interface from the drivers and/or userspace,
> because you want access to the whole region to properly handle the
> device, no?
> 
> IOW, with the current changes, isn't the functionality broken for
> non page-aligned addr and/or size?

-- 
Petr Matousek / Red Hat Security Response Team
PGP: 0xC44977CA 8107 AF16 A416 F9AF 18F3  D874 3E78 6F42 C449 77CA
[prev in list] [next in list] [prev in thread] [next in thread]