Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1321801 > unrolled thread

Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and  track_pfn_remap() Matthew Wilcox <willy@linux.intel.com> - 2016-01-29 15:50 +0100
    Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Andy Lutomirski <luto@amacapital.net> - 2016-01-29 23:20 +0100
    Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and  track_pfn_remap() Ingo Molnar <mingo@kernel.org> - 2016-02-09 15:30 +0100
      Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Andy Lutomirski <luto@amacapital.net> - 2016-02-10 04:10 +0100

#1321801 — Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

On Tue, Jan 26, 2016 at 09:44:24PM -0800, Andy Lutomirski wrote:
> On Tue, Jan 26, 2016 at 8:40 PM, Matthew Wilcox <willy@linux.intel.com> wrote:
> > On Mon, Jan 25, 2016 at 09:33:35AM -0800, Andy Lutomirski wrote:
> >> On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
> >> <matthew.r.wilcox@intel.com> wrote:
> >> > From: Matthew Wilcox <willy@linux.intel.com>
> >> >
> >> > track_pfn_insert() overwrites the pgprot that is passed in with a value
> >> > based on the VMA's page_prot.  This is a problem for people trying to
> >> > do clever things with the new vm_insert_pfn_prot() as it will simply
> >> > overwrite the passed protection flags.  If we use the current value of
> >> > the pgprot as the base, then it will behave as people are expecting.
> >> >
> >> > Also fix track_pfn_remap() in the same way.
> >>
> >> Well that's embarrassing.  Presumably it worked for me because I only
> >> overrode the cacheability bits and lookup_memtype did the right thing.
> >>
> >> But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
> >> requests it?  Or are there no callers that actually need that?  (HPET
> >> doesn't, because there's a plain old ioremapped mapping.)
> >
> > I'm confused.  Here's what I understand:
> >
> >  - on x86, the bits in pgprot can be considered as two sets of bits;
> >    the 'cacheability bits' -- those in _PAGE_CACHE_MASK and the
> >    'protection bits' -- PRESENT, RW, USER, ACCESSED, NX
> >  - The purpose of track_pfn_insert() is to ensure that the cacheability bits
> >    are the same on all mappings of a given page, as strongly advised by the
> >    Intel manuals [1].  So track_pfn_insert() is really only supposed to
> >    modify _PAGE_CACHE_MASK of the passed pgprot, but in fact it ends up
> >    modifying the protection bits as well, due to the bug.
> >
> > I don't think you overrode the cacheability bits at all.  It looks to
> > me like your patch ends up mapping the HPET into userspace writable.
> 
> I sure hope not.  If vm_page_prot was writable, something was already
> broken, because this is the vvar mapping, and the vvar mapping is
> VM_READ (and not even VM_MAYREAD).

I do beg yor pardon.  I thought you were inserting a readonly page
into the middle of a writable mapping.  Instead you're inserting a
non-executable page into the middle of a VM_READ | VM_EXEC mapping.
Sorry for the confusion.  I should have written:

"like your patch ends up mapping the HPET into userspace executable"

which is far less exciting.

> > I don't think the vm_insert_pfn_prot() call gets to change the memtype.
> > For one, that page may already be mapped into a differet userspace using
> > the pre-existing memtype, and [1] continues to bite you.  Then there
> > may be outstanding kernel users of the page that's being mapped in.
> 
> So why was remap_pfn_range different?  I'm sure there was a reason.

Yeah, doesn't make sense to me either.

[toc] | [next] | [standalone]

#1322125 — Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

On Fri, Jan 29, 2016 at 6:49 AM, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Tue, Jan 26, 2016 at 09:44:24PM -0800, Andy Lutomirski wrote:
>> On Tue, Jan 26, 2016 at 8:40 PM, Matthew Wilcox <willy@linux.intel.com> wrote:
>> > On Mon, Jan 25, 2016 at 09:33:35AM -0800, Andy Lutomirski wrote:
>> >> On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
>> >> <matthew.r.wilcox@intel.com> wrote:
>> >> > From: Matthew Wilcox <willy@linux.intel.com>
>> >> >
>> >> > track_pfn_insert() overwrites the pgprot that is passed in with a value
>> >> > based on the VMA's page_prot.  This is a problem for people trying to
>> >> > do clever things with the new vm_insert_pfn_prot() as it will simply
>> >> > overwrite the passed protection flags.  If we use the current value of
>> >> > the pgprot as the base, then it will behave as people are expecting.
>> >> >
>> >> > Also fix track_pfn_remap() in the same way.
>> >>
>> >> Well that's embarrassing.  Presumably it worked for me because I only
>> >> overrode the cacheability bits and lookup_memtype did the right thing.
>> >>
>> >> But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
>> >> requests it?  Or are there no callers that actually need that?  (HPET
>> >> doesn't, because there's a plain old ioremapped mapping.)
>> >
>> > I'm confused.  Here's what I understand:
>> >
>> >  - on x86, the bits in pgprot can be considered as two sets of bits;
>> >    the 'cacheability bits' -- those in _PAGE_CACHE_MASK and the
>> >    'protection bits' -- PRESENT, RW, USER, ACCESSED, NX
>> >  - The purpose of track_pfn_insert() is to ensure that the cacheability bits
>> >    are the same on all mappings of a given page, as strongly advised by the
>> >    Intel manuals [1].  So track_pfn_insert() is really only supposed to
>> >    modify _PAGE_CACHE_MASK of the passed pgprot, but in fact it ends up
>> >    modifying the protection bits as well, due to the bug.
>> >
>> > I don't think you overrode the cacheability bits at all.  It looks to
>> > me like your patch ends up mapping the HPET into userspace writable.
>>
>> I sure hope not.  If vm_page_prot was writable, something was already
>> broken, because this is the vvar mapping, and the vvar mapping is
>> VM_READ (and not even VM_MAYREAD).
>
> I do beg yor pardon.  I thought you were inserting a readonly page
> into the middle of a writable mapping.  Instead you're inserting a
> non-executable page into the middle of a VM_READ | VM_EXEC mapping.
> Sorry for the confusion.  I should have written:
>
> "like your patch ends up mapping the HPET into userspace executable"
>
> which is far less exciting.

I think it's not even that.  That particular mapping is just VM_READ.

Anyway, this patch is:

Acked-by: Andy Lutomirski <luto@kernel.org>

Ingo etc: this patch should probably go in to tip:x86/asm -- the code
currently in there is wrong, even if it has no obvious symptom.

--Andy

[toc] | [prev] | [next] | [standalone]

#1330281

* Matthew Wilcox <willy@linux.intel.com> wrote:

> > I sure hope not.  If vm_page_prot was writable, something was already broken, 
> > because this is the vvar mapping, and the vvar mapping is VM_READ (and not 
> > even VM_MAYREAD).
> 
> I do beg yor pardon.  I thought you were inserting a readonly page into the 
> middle of a writable mapping.  Instead you're inserting a non-executable page 
> into the middle of a VM_READ | VM_EXEC mapping. Sorry for the confusion.  I 
> should have written:
> 
> "like your patch ends up mapping the HPET into userspace executable"
> 
> which is far less exciting.

Btw., a side note, an executable HPET page has its own dangers as well, for 
example because it always changes in value, it can probabilistically represent 
'sensible' (and dangerous) executable x86 instructions that exploits can return 
to.

So only mapping it readable (which Andy's patch attempts I think) is worthwile.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1330871 — Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

On Feb 9, 2016 6:24 AM, "Ingo Molnar" <mingo@kernel.org> wrote:
>
>
> * Matthew Wilcox <willy@linux.intel.com> wrote:
>
> > > I sure hope not.  If vm_page_prot was writable, something was already broken,
> > > because this is the vvar mapping, and the vvar mapping is VM_READ (and not
> > > even VM_MAYREAD).
> >
> > I do beg yor pardon.  I thought you were inserting a readonly page into the
> > middle of a writable mapping.  Instead you're inserting a non-executable page
> > into the middle of a VM_READ | VM_EXEC mapping. Sorry for the confusion.  I
> > should have written:
> >
> > "like your patch ends up mapping the HPET into userspace executable"
> >
> > which is far less exciting.
>
> Btw., a side note, an executable HPET page has its own dangers as well, for
> example because it always changes in value, it can probabilistically represent
> 'sensible' (and dangerous) executable x86 instructions that exploits can return
> to.
>
> So only mapping it readable (which Andy's patch attempts I think) is worthwile.

The whole vma is readable but not executable, so I don't think this
was a problem.  It's also at a randomized address, which helps.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web