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

Groups > linux.kernel > #1441517 > unrolled thread

Re: Minor PKRU bug?

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: Minor PKRU bug? Dave Hansen <dave.hansen@linux.intel.com> - 2016-07-12 17:40 +0200
    Re: Minor PKRU bug? "H. Peter Anvin" <hpa@zytor.com> - 2016-07-13 01:00 +0200
      Re: Minor PKRU bug? Dave Hansen <dave.hansen@linux.intel.com> - 2016-07-13 01:10 +0200
      Re: Minor PKRU bug? Andy Lutomirski <luto@amacapital.net> - 2016-07-13 01:10 +0200
        Re: Minor PKRU bug? Dave Hansen <dave.hansen@linux.intel.com> - 2016-07-21 23:40 +0200
          Re: Minor PKRU bug? Andy Lutomirski <luto@amacapital.net> - 2016-07-21 23:50 +0200
            Re: Minor PKRU bug? "H. Peter Anvin" <hpa@zytor.com> - 2016-07-22 00:00 +0200
              Re: Minor PKRU bug? Dave Hansen <dave.hansen@linux.intel.com> - 2016-07-22 00:30 +0200

#1441517 — Re: Minor PKRU bug?

On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
> is_prefetch in arch/x86/mm/fault.c can be called on a user address
> that's not readable due to PKRU.  This could break it.  You might need
> to add a get_user_exec or similar.

Thanks for the heads-up.  I think I'll just need a version that does
something along the lines of stac/clac, but with PKRU.

I think I can do it with an "_exec" variant of probe_kernel_address(),
but it's a bit messy.

[toc] | [next] | [standalone]

#1441863

On 07/12/16 08:32, Dave Hansen wrote:
> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>> is_prefetch in arch/x86/mm/fault.c can be called on a user address
>> that's not readable due to PKRU.  This could break it.  You might need
>> to add a get_user_exec or similar.
> 
> Thanks for the heads-up.  I think I'll just need a version that does
> something along the lines of stac/clac, but with PKRU.
> 
> I think I can do it with an "_exec" variant of probe_kernel_address(),
> but it's a bit messy.
> 

Can this particular codepath even be executed on a PKRU-equipped
machine?  I thought it was a bug fix for a specific AMD CPU line.

	-hpa

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

#1441896

On 07/12/2016 03:55 PM, H. Peter Anvin wrote:
> On 07/12/16 08:32, Dave Hansen wrote:
>> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>>> is_prefetch in arch/x86/mm/fault.c can be called on a user address
>>> that's not readable due to PKRU.  This could break it.  You might need
>>> to add a get_user_exec or similar.
>>
>> Thanks for the heads-up.  I think I'll just need a version that does
>> something along the lines of stac/clac, but with PKRU.
>>
>> I think I can do it with an "_exec" variant of probe_kernel_address(),
>> but it's a bit messy.
> 
> Can this particular codepath even be executed on a PKRU-equipped
> machine?  I thought it was a bug fix for a specific AMD CPU line.

Yeah, I think we hit it unconditionally in the naughty paths of the page
fault handler.  I don't see any CPU model detection in the call path.  I
think we just assume that everybody has prefetch bugs.

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

#1441897

On Tue, Jul 12, 2016 at 3:55 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/12/16 08:32, Dave Hansen wrote:
>> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>>> is_prefetch in arch/x86/mm/fault.c can be called on a user address
>>> that's not readable due to PKRU.  This could break it.  You might need
>>> to add a get_user_exec or similar.
>>
>> Thanks for the heads-up.  I think I'll just need a version that does
>> something along the lines of stac/clac, but with PKRU.
>>
>> I think I can do it with an "_exec" variant of probe_kernel_address(),
>> but it's a bit messy.
>>
>
> Can this particular codepath even be executed on a PKRU-equipped
> machine?  I thought it was a bug fix for a specific AMD CPU line.

It can certainly be executed -- do_sigbus will execute it every time.
But I guess it doesn't matter if it fails on a PKRU machine, because a
failure will just report the signal, and the erratum case can't happen
in the first place.

>
>         -hpa
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1448191

[Multipart message — attachments visible in raw view] — view raw

On 07/12/2016 03:59 PM, Andy Lutomirski wrote:
> On Tue, Jul 12, 2016 at 3:55 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 07/12/16 08:32, Dave Hansen wrote:
>>> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>>>> is_prefetch in arch/x86/mm/fault.c can be called on a user address
>>>> that's not readable due to PKRU.  This could break it.  You might need
>>>> to add a get_user_exec or similar.
>>>
>>> Thanks for the heads-up.  I think I'll just need a version that does
>>> something along the lines of stac/clac, but with PKRU.
>>>
>>> I think I can do it with an "_exec" variant of probe_kernel_address(),
>>> but it's a bit messy.
>>>
>> Can this particular codepath even be executed on a PKRU-equipped
>> machine?  I thought it was a bug fix for a specific AMD CPU line.
> 
> It can certainly be executed -- do_sigbus will execute it every time.
> But I guess it doesn't matter if it fails on a PKRU machine, because a
> failure will just report the signal, and the erratum case can't happen
> in the first place.

Hi Andy,

I look at it this way:

Systems without prefetch errata always see is_prefetch() return false.
If is_prefetch() faults when trying to fetch an instruction it returns
false.  Protection keys will make it do this.

Essentially, any pkeys-execute-only code can not have prefetch errata
detected inside it.  Any future processor with such an erratum will need
a different workaround.

What do folks think?  Is it worth shoring this up in case of a future
erratum?

The patch to fix it isn't too invasive (attached).

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

#1448199

On Thu, Jul 21, 2016 at 2:35 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 07/12/2016 03:59 PM, Andy Lutomirski wrote:
>> On Tue, Jul 12, 2016 at 3:55 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 07/12/16 08:32, Dave Hansen wrote:
>>>> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>>>>> is_prefetch in arch/x86/mm/fault.c can be called on a user address
>>>>> that's not readable due to PKRU.  This could break it.  You might need
>>>>> to add a get_user_exec or similar.
>>>>
>>>> Thanks for the heads-up.  I think I'll just need a version that does
>>>> something along the lines of stac/clac, but with PKRU.
>>>>
>>>> I think I can do it with an "_exec" variant of probe_kernel_address(),
>>>> but it's a bit messy.
>>>>
>>> Can this particular codepath even be executed on a PKRU-equipped
>>> machine?  I thought it was a bug fix for a specific AMD CPU line.
>>
>> It can certainly be executed -- do_sigbus will execute it every time.
>> But I guess it doesn't matter if it fails on a PKRU machine, because a
>> failure will just report the signal, and the erratum case can't happen
>> in the first place.
>
> Hi Andy,
>
> I look at it this way:
>
> Systems without prefetch errata always see is_prefetch() return false.
> If is_prefetch() faults when trying to fetch an instruction it returns
> false.  Protection keys will make it do this.
>
> Essentially, any pkeys-execute-only code can not have prefetch errata
> detected inside it.  Any future processor with such an erratum will need
> a different workaround.
>
> What do folks think?  Is it worth shoring this up in case of a future
> erratum?
>
> The patch to fix it isn't too invasive (attached).

I like it, except that reading just a single byte is a bit silly.
OTOH, that's what the current code needs and I see no fundamental
reason to change it until there's a real user.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1448207

On July 21, 2016 2:45:49 PM PDT, Andy Lutomirski <luto@amacapital.net> wrote:
>On Thu, Jul 21, 2016 at 2:35 PM, Dave Hansen
><dave.hansen@linux.intel.com> wrote:
>> On 07/12/2016 03:59 PM, Andy Lutomirski wrote:
>>> On Tue, Jul 12, 2016 at 3:55 PM, H. Peter Anvin <hpa@zytor.com>
>wrote:
>>>> On 07/12/16 08:32, Dave Hansen wrote:
>>>>> On 07/09/2016 02:27 PM, Andy Lutomirski wrote:
>>>>>> is_prefetch in arch/x86/mm/fault.c can be called on a user
>address
>>>>>> that's not readable due to PKRU.  This could break it.  You might
>need
>>>>>> to add a get_user_exec or similar.
>>>>>
>>>>> Thanks for the heads-up.  I think I'll just need a version that
>does
>>>>> something along the lines of stac/clac, but with PKRU.
>>>>>
>>>>> I think I can do it with an "_exec" variant of
>probe_kernel_address(),
>>>>> but it's a bit messy.
>>>>>
>>>> Can this particular codepath even be executed on a PKRU-equipped
>>>> machine?  I thought it was a bug fix for a specific AMD CPU line.
>>>
>>> It can certainly be executed -- do_sigbus will execute it every
>time.
>>> But I guess it doesn't matter if it fails on a PKRU machine, because
>a
>>> failure will just report the signal, and the erratum case can't
>happen
>>> in the first place.
>>
>> Hi Andy,
>>
>> I look at it this way:
>>
>> Systems without prefetch errata always see is_prefetch() return
>false.
>> If is_prefetch() faults when trying to fetch an instruction it
>returns
>> false.  Protection keys will make it do this.
>>
>> Essentially, any pkeys-execute-only code can not have prefetch errata
>> detected inside it.  Any future processor with such an erratum will
>need
>> a different workaround.
>>
>> What do folks think?  Is it worth shoring this up in case of a future
>> erratum?
>>
>> The patch to fix it isn't too invasive (attached).
>
>I like it, except that reading just a single byte is a bit silly.
>OTOH, that's what the current code needs and I see no fundamental
>reason to change it until there's a real user.
>
>--Andy

The thing is that we can't actually test this, since there is no machine on which this code path will ever execute.  That concerns me a bit.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

#1448228

On 07/21/2016 02:48 PM, H. Peter Anvin wrote:
>> >I like it, except that reading just a single byte is a bit silly.
>> >OTOH, that's what the current code needs and I see no fundamental
>> >reason to change it until there's a real user.
>>> 
> The thing is that we can't actually test this, since there is no
> machine on which this code path will ever execute.  That concerns me
> a bit.

I rigged the is_prefetch() check to return true on an instruction that I
know causes a sigbus.  If I run without protection keys, this setup sits
in a never-ending fault loop, which is the behavior that we want from
*real* prefetch instructions.

But, if I have that instruction be marked execute-only by pkeys,
is_prefetch() returns false and the app gets the sigbus, and it *looks*
like it came from the (fake) prefetch instruction, which isn't what we want.

It's not exactly a real-world test, but it did convince me that the code
is doing the right thing.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web