> On 14-Jul-2020, at 11:38 AM, Michael Ellerman <m...@ellerman.id.au> wrote: > > Athira Rajeev <atraj...@linux.vnet.ibm.com > <mailto:atraj...@linux.vnet.ibm.com>> writes: >>> On 19-Mar-2020, at 4:22 PM, Michael Ellerman <m...@ellerman.id.au> wrote: >>> >>> Hi Athira, >>> >>> Athira Rajeev <atraj...@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