lkml.org 
[lkml]   [2012]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: mtd: kernel BUG at arch/x86/mm/pat.c:279!
    From
    Date
    On Fri, 2012-09-07 at 16:09 -0700, Linus Torvalds wrote:
    > The "u32 len" -> "unsigned long len" thing *might* make a difference, though.

    This I believe doesn't fix the reported BUG. I was trying to address
    your previous comment about broken types.

    >
    > I also think your patch is incomplete even on 32-bit, because this:
    >
    > > if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
    > > off = vma->vm_pgoff << PAGE_SHIFT;
    >
    > is still wrong. It probably should be
    >
    > off = vma->vm_pgoff;
    > off <<= PAGE_SHIFT;
    >
    > because vm_pgoff may be a 32-bit type, while "resource_size_t" may be
    > 64-bit. Shifting the 32-bit type without a cast (implicit or explicit)
    > isn't going to help.

    Agree.

    > That said, we have absolutely *tons* of bugs with this particular
    > pattern. Just do
    >
    > git grep 'vm_pgoff.*<<.*PAGE_SHIFT'
    >
    > and there are distressingly few casts in there (there's a few, mainly
    > in fs/proc).
    >
    > Now, I suspect many of them are fine just because most users probably
    > are size-limited anyway, but it's a bit distressing stuff. And I
    > suspect it means we might want to introduce a helper function like
    >
    > static inline u64 vm_offset(struct vm_area_struct *vma)
    > {
    > return (u64)vma->vm_pgoff << PAGE_SHIFT;
    > }
    >
    > or something. Maybe add the "vm_length()" helper while at it too,
    > since the whole "vma->vm_end - vma->vm_start" thing is so common.

    Agree.

    > Anyway, since Sasha's oops is clearly not 32-bit, the above issues
    > don't matter, and it would be interesting to hear if it's the 32-bit
    > 'len' thing that triggers this problem. Still, I can't see how it
    > would - as far as I can tell, a truncated 'len' would at most result
    > in spurious early "return -EINVAL", not any real problem.
    >
    > What are we missing?
    >

    On Fri, 2012-09-07 at 15:42 -0700, Suresh Siddha wrote:
    > - if ((vma->vm_end - vma->vm_start + off) > len)
    > + if (off >= len || (vma->vm_end - vma->vm_start + off) > len)
    > return -EINVAL;

    This is the relevant portion that I am thinking will address the BUG.

    Essentially the user is trying to mmap at a very large offset (from the
    oops it appears "vma->vm_pgoff << PAGE_SHIFT + start" ends up to
    "0xfffffffffffff000").

    So it appears that the condition "(vma->vm_end - vma->vm_start + off) >
    len" might be false because of the wraparound? and doesn't return
    -EINVAL.

    Let's see what Sasha finds. Anyways the patch does indeed require your
    above mentioned vm_pgoff fix for the 32-bit case.

    thanks,
    suresh





    \
     
     \ /
      Last update: 2012-09-08 02:41    [W:0.051 / U:30.620 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site