linux-kernel - Re: [PATCH 4.1 11/86] iommu/amd: Fix BUG when faulting a PROT

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, 6 Nov 2015 12:49:20 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	stable <stable@...r.kernel.org>, Jay Cornwall <jay@...rnwall.me>,
	Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH 4.1 11/86] iommu/amd: Fix BUG when faulting a PROT_NONE VMA

On Fri, Nov 6, 2015 at 11:22 AM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> From: Jay Cornwall <jay@...rnwall.me>
>
> commit d14f6fced5f9360edca5a1325ddb7077aab1203b upstream.
>
> handle_mm_fault indirectly triggers a BUG in do_numa_page
> when given a VMA without read/write/execute access. Check
> this condition in do_fault.

This reminds me.

I think the code is still wrong.

The thing is, the VM assumes that the caller has already checked
permissions. An dby "checked permissions", I mean actually checking
permissions. The AMD iommu driver doesn't do that, it does something
completely different, namely "check it's not PROT_NONE".

So I think the code should instead do something like

   if ((write && !(vma->vm_flags & VM_WRITE)) || !(vma->vm_flags & VM_READ)) {
      up_read(&mm->mmap_sem);
      handle_fault_error(fault);
      goto out;
   }

because it is *not* valid to call "handle_mm_fault()" with a write
fault unless you have write permissions (or with a read fault unless
you have read permissions).

And some "handle_mm_fault would BUG_ON()" comment is just bogus. It's
not handle_mm_fault()'s case that you called it without checking
proper permissions.

I'm not arguing against the stable backport, because that is fine. But
I think this should be fixed further.

Joerg?

                Linus
--
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/