| Message ID | 1584533181-4331-1-git-send-email-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] powerpc/perf: Use SIER_USER_MASK while updating SPRN_SIER for EBB events | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (8a445cbcb9f5090cb07ec6cbb89a8a1fc99a0ff7) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
Hi Athira, Athira Rajeev <atrajeev@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? cheers > Fixes: 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit book3s") > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > --- > Changelog: > v2 -> v3: > - Corrected name of SIER register in commit message > as pointed by Segher Boessenkool > v1 -> v2: > - Make the commit message more clearer. > > arch/powerpc/perf/core-book3s.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 3086055..48b61cc 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -579,7 +579,7 @@ static void ebb_switch_out(unsigned long mmcr0) > return; > > current->thread.siar = mfspr(SPRN_SIAR); > - current->thread.sier = mfspr(SPRN_SIER); > + current->thread.sier = mfspr(SPRN_SIER) & SIER_USER_MASK; > current->thread.sdar = mfspr(SPRN_SDAR); > current->thread.mmcr0 = mmcr0 & MMCR0_USER_MASK; > current->thread.mmcr2 = mfspr(SPRN_MMCR2) & MMCR2_USER_MASK; > -- > 1.8.3.1
> On 19-Mar-2020, at 4:22 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > Hi Athira, > > Athira Rajeev <atrajeev@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. In that case, Can we drop the existing definition of SIER_USER_MASK if it is no more needed ? Thanks Athira > > cheers > >> Fixes: 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit book3s") >> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> >> --- >> Changelog: >> v2 -> v3: >> - Corrected name of SIER register in commit message >> as pointed by Segher Boessenkool >> v1 -> v2: >> - Make the commit message more clearer. >> >> arch/powerpc/perf/core-book3s.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c >> index 3086055..48b61cc 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -579,7 +579,7 @@ static void ebb_switch_out(unsigned long mmcr0) >> return; >> >> current->thread.siar = mfspr(SPRN_SIAR); >> - current->thread.sier = mfspr(SPRN_SIER); >> + current->thread.sier = mfspr(SPRN_SIER) & SIER_USER_MASK; >> current->thread.sdar = mfspr(SPRN_SDAR); >> current->thread.mmcr0 = mmcr0 & MMCR0_USER_MASK; >> current->thread.mmcr2 = mfspr(SPRN_MMCR2) & MMCR2_USER_MASK; >> -- >> 1.8.3.1
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes: >> On 19-Mar-2020, at 4:22 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: >> >> Hi Athira, >> >> Athira Rajeev <atrajeev@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 :) > 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
> On 14-Jul-2020, at 11:38 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > Athira Rajeev <atrajeev@linux.vnet.ibm.com <mailto:atrajeev@linux.vnet.ibm.com>> writes: >>> On 19-Mar-2020, at 4:22 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: >>> >>> Hi Athira, >>> >>> Athira Rajeev <atrajeev@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
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index 3086055..48b61cc 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -579,7 +579,7 @@ static void ebb_switch_out(unsigned long mmcr0) return; current->thread.siar = mfspr(SPRN_SIAR); - current->thread.sier = mfspr(SPRN_SIER); + current->thread.sier = mfspr(SPRN_SIER) & SIER_USER_MASK; current->thread.sdar = mfspr(SPRN_SDAR); current->thread.mmcr0 = mmcr0 & MMCR0_USER_MASK; current->thread.mmcr2 = mfspr(SPRN_MMCR2) & MMCR2_USER_MASK;
Sampled Instruction Event Register (SIER), is a PMU register, captures architecture state for a given sample. And 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. 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. Fixes: 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit book3s") Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- Changelog: v2 -> v3: - Corrected name of SIER register in commit message as pointed by Segher Boessenkool v1 -> v2: - Make the commit message more clearer. arch/powerpc/perf/core-book3s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)