[PATCH v3] powerpc/perf: Use SIER_USER_MASK while updating SPRN_SIER for EBB events

[Athira Rajeev]

> On 14-Jul-2020, at 11:38 AM, Michael Ellerman <mpe at ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev at linux.vnet.ibm.com <mailto:atrajeev at linux.vnet.ibm.com>> writes:
>>> On 19-Mar-2020, at 4:22 PM, Michael Ellerman <mpe at ellerman.id.au> wrote:
>>> 
>>> Hi Athira,
>>> 
>>> Athira Rajeev <atrajeev at linux.vnet.ibm.com> writes:
>>>> Sampled Instruction Event Register (SIER), is a PMU register,
>>>                                                              ^
>>>                                                              that
>>>> captures architecture state for a given sample. And sier_user_mask
>>>          ^                                          ^
>>>          don't think we need "architecture"         SIER_USER_MASK
>>> 
>>>> defined in commit 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit
>>>> book3s") defines the architected bits that needs to be saved from the SPR.
>>> 
>>> Not quite, it defines the bits that are visible to userspace.
>>> 
>>> And I think it's true that for EBB events the bits we need/want to save
>>> are only the user visible bits.
>>> 
>>>> Currently all of the bits from SIER are saved for EBB events. Patch fixes
>>>> this by ANDing the "sier_user_mask" to data from SIER in ebb_switch_out().
>>>> This will force save only architected bits from the SIER.
>>> 
>>> s/architected/user visible/
>>> 
>>> 
>>> But, why does it matter? The kernel saves the user visible bits, as well
>>> as the kernel-only bits into the thread struct. And then later the
>>> kernel restores that value into the hardware before returning to
>>> userspace.
>>> 
>>> But the hardware enforces the visibility of the bits, so userspace can't
>>> observe any bits that it shouldn't.
>>> 
>>> Or is there some other mechanism whereby userspace can see those bits? ;)
>>> 
>>> If there was, what would the security implications of that be?
>> 
>> Hi Michael,
>> 
>> Thanks for your comments. 
>> 
>> In ebb_switch_in, we set PMCC bit [MMCR0 44:45 ] to 10 which means
>> SIER ( Group B ) register is readable in problem state. Hence the
>> intention of the patch was to make sure we are not exposing the bits
>> which the userspace shouldn't be reading.
>> 
>> But following your comment about "hardware enforcing the visibility of
>> bits", I did try an "ebb" experiment which showed that reading
>> SPRN_SIER didn't expose any bits other than the user visible bits.
>> Sorry for the confusion here.
> 
> That's OK. Thanks for following my trail of clues :)
> 

Sure, Thanks for your inputs. I will come back on this after checking on information exposed via Ptrace

>> In that case, Can we drop the existing definition of SIER_USER_MASK if
>> it is no more needed ?
> 
> I think it is still needed, and I think this change to use it is good, because
> SIER is visible via ptrace.
> 
> What we need to do, is look at what information in SIER we are currently
> exposing to userspace via ptrace, and what the security implications (if
> any) of that are.
> 
> cheers

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20200715/9f4180d0/attachment.htm>