linux-kernel - Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!

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:	Fri, 07 Sep 2012 16:54:05 -0700
From:	Suresh Siddha <suresh.b.siddha@...el.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Sasha Levin <levinsasha928@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, dwmw2@...radead.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-mtd@...ts.infradead.org, linux-mm <linux-mm@...ck.org>,
	Dave Jones <davej@...hat.com>
Subject: Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/