* [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer @ 2018-03-04 11:55 Madhavan Srinivasan 2018-03-04 11:55 ` [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR Madhavan Srinivasan 2018-03-05 6:16 ` [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Balbir Singh 0 siblings, 2 replies; 6+ messages in thread From: Madhavan Srinivasan @ 2018-03-04 11:55 UTC (permalink / raw) To: mpe; +Cc: linuxppc-dev, Madhavan Srinivasan The current Branch History Rolling Buffer (BHRB) code does not check for any privilege levels before updating the data from BHRB. This leaks kernel addresses to userspace even when profiling only with userspace privileges. Add proper checks to prevent it. Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- arch/powerpc/perf/core-book3s.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index f89bbd54ecec..337db5831749 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -457,6 +457,10 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw) /* invalid entry */ continue; + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && + is_kernel_addr(addr)) + continue; + /* Branches are read most recent first (ie. mfbhrb 0 is * the most recent branch). * There are two types of valid entries: -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR 2018-03-04 11:55 [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Madhavan Srinivasan @ 2018-03-04 11:55 ` Madhavan Srinivasan 2018-03-05 8:21 ` Naveen N. Rao 2018-03-05 6:16 ` [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Balbir Singh 1 sibling, 1 reply; 6+ messages in thread From: Madhavan Srinivasan @ 2018-03-04 11:55 UTC (permalink / raw) To: mpe; +Cc: linuxppc-dev, Madhavan Srinivasan Sampled Data Address Register (SDAR) is a 64-bit register that contains the effective address of the storage operand of an instruction that was being executed, possibly out-of-order, at or around the time that the Performance Monitor alert occurred. In certain scenario SDAR happen to contain the kernel address even for userspace only sampling. Add checks to prevent it. Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- arch/powerpc/perf/core-book3s.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index 337db5831749..c4525323d691 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -95,7 +95,7 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs) { return 0; } -static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) { } +static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, struct perf_event *event) { } static inline u32 perf_get_misc_flags(struct pt_regs *regs) { return 0; @@ -174,7 +174,7 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs) * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC, the * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in SIER. */ -static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) +static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, struct perf_event *event) { unsigned long mmcra = regs->dsisr; bool sdar_valid; @@ -198,6 +198,11 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid) *addrp = mfspr(SPRN_SDAR); + + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && + (event->attr.exclude_kernel || event->attr.exclude_hv) && + is_kernel_addr(mfspr(SPRN_SDAR))) + *addrp = 0; } static bool regs_sihv(struct pt_regs *regs) @@ -2054,7 +2059,7 @@ static void record_and_restart(struct perf_event *event, unsigned long val, if (event->attr.sample_type & (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR)) - perf_get_data_addr(regs, &data.addr); + perf_get_data_addr(regs, &data.addr, event); if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) { struct cpu_hw_events *cpuhw; -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR 2018-03-04 11:55 ` [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR Madhavan Srinivasan @ 2018-03-05 8:21 ` Naveen N. Rao 2018-03-07 4:53 ` Madhavan Srinivasan 0 siblings, 1 reply; 6+ messages in thread From: Naveen N. Rao @ 2018-03-05 8:21 UTC (permalink / raw) To: Madhavan Srinivasan, mpe; +Cc: linuxppc-dev Madhavan Srinivasan wrote: > Sampled Data Address Register (SDAR) is a 64-bit > register that contains the effective address of > the storage operand of an instruction that was > being executed, possibly out-of-order, at or around > the time that the Performance Monitor alert occurred. >=20 > In certain scenario SDAR happen to contain the kernel > address even for userspace only sampling. Add checks > to prevent it. >=20 > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > --- > arch/powerpc/perf/core-book3s.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) >=20 > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-boo= k3s.c > index 337db5831749..c4525323d691 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -95,7 +95,7 @@ static inline unsigned long perf_ip_adjust(struct pt_re= gs *regs) > { > return 0; > } > -static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) = { } > +static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, = struct perf_event *event) { } > static inline u32 perf_get_misc_flags(struct pt_regs *regs) > { > return 0; > @@ -174,7 +174,7 @@ static inline unsigned long perf_ip_adjust(struct pt_= regs *regs) > * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC, t= he > * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in SIE= R. > */ > -static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) > +static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, = struct perf_event *event) > { > unsigned long mmcra =3D regs->dsisr; > bool sdar_valid; > @@ -198,6 +198,11 @@ static inline void perf_get_data_addr(struct pt_regs= *regs, u64 *addrp) >=20 > if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid) > *addrp =3D mfspr(SPRN_SDAR); > + > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && > + (event->attr.exclude_kernel || event->attr.exclude_hv) && I may be missing something, but if !capable(CAP_SYS_ADMIN), should we=20 still check the exclude_kernel/exclude_hv fields in the event attribute? =20 Aren't those user controlled? - Naveen > + is_kernel_addr(mfspr(SPRN_SDAR))) > + *addrp =3D 0; > } >=20 > static bool regs_sihv(struct pt_regs *regs) > @@ -2054,7 +2059,7 @@ static void record_and_restart(struct perf_event *e= vent, unsigned long val, >=20 > if (event->attr.sample_type & > (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR)) > - perf_get_data_addr(regs, &data.addr); > + perf_get_data_addr(regs, &data.addr, event); >=20 > if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) { > struct cpu_hw_events *cpuhw; > --=20 > 2.7.4 >=20 >=20 = ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR 2018-03-05 8:21 ` Naveen N. Rao @ 2018-03-07 4:53 ` Madhavan Srinivasan 0 siblings, 0 replies; 6+ messages in thread From: Madhavan Srinivasan @ 2018-03-07 4:53 UTC (permalink / raw) To: Naveen N. Rao, mpe; +Cc: linuxppc-dev On Monday 05 March 2018 01:51 PM, Naveen N. Rao wrote: > Madhavan Srinivasan wrote: >> Sampled Data Address Register (SDAR) is a 64-bit >> register that contains the effective address of >> the storage operand of an instruction that was >> being executed, possibly out-of-order, at or around >> the time that the Performance Monitor alert occurred. >> >> In certain scenario SDAR happen to contain the kernel >> address even for userspace only sampling. Add checks >> to prevent it. >> >> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> --- >> arch/powerpc/perf/core-book3s.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/perf/core-book3s.c >> b/arch/powerpc/perf/core-book3s.c >> index 337db5831749..c4525323d691 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -95,7 +95,7 @@ static inline unsigned long perf_ip_adjust(struct >> pt_regs *regs) >> { >> return 0; >> } >> -static inline void perf_get_data_addr(struct pt_regs *regs, u64 >> *addrp) { } >> +static inline void perf_get_data_addr(struct pt_regs *regs, u64 >> *addrp, struct perf_event *event) { } >> static inline u32 perf_get_misc_flags(struct pt_regs *regs) >> { >> return 0; >> @@ -174,7 +174,7 @@ static inline unsigned long perf_ip_adjust(struct >> pt_regs *regs) >> * pointed to by SIAR; this is indicated by the >> [POWER6_]MMCRA_SDSYNC, the >> * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in >> SIER. >> */ >> -static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) >> +static inline void perf_get_data_addr(struct pt_regs *regs, u64 >> *addrp, struct perf_event *event) >> { >> unsigned long mmcra = regs->dsisr; >> bool sdar_valid; >> @@ -198,6 +198,11 @@ static inline void perf_get_data_addr(struct >> pt_regs *regs, u64 *addrp) >> >> if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid) >> *addrp = mfspr(SPRN_SDAR); >> + >> + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && >> + (event->attr.exclude_kernel || event->attr.exclude_hv) && > > I may be missing something, but if !capable(CAP_SYS_ADMIN), should we > still check the exclude_kernel/exclude_hv fields in the event > attribute? Aren't those user controlled? > Yes that right. But i also want to handle the case when we sampling only for userspace even with higher privilege level. May be I should handle that as a separate patch. I will respin this patch to check only for the privilege level and change the commit message accordingly. Thanks for review Maddy > - Naveen > >> + is_kernel_addr(mfspr(SPRN_SDAR))) >> + *addrp = 0; >> } >> >> static bool regs_sihv(struct pt_regs *regs) >> @@ -2054,7 +2059,7 @@ static void record_and_restart(struct >> perf_event *event, unsigned long val, >> >> if (event->attr.sample_type & >> (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR)) >> - perf_get_data_addr(regs, &data.addr); >> + perf_get_data_addr(regs, &data.addr, event); >> >> if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) { >> struct cpu_hw_events *cpuhw; >> -- >> 2.7.4 >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer 2018-03-04 11:55 [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Madhavan Srinivasan 2018-03-04 11:55 ` [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR Madhavan Srinivasan @ 2018-03-05 6:16 ` Balbir Singh 2018-03-07 4:54 ` Madhavan Srinivasan 1 sibling, 1 reply; 6+ messages in thread From: Balbir Singh @ 2018-03-05 6:16 UTC (permalink / raw) To: Madhavan Srinivasan Cc: Michael Ellerman, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT) On Sun, Mar 4, 2018 at 10:55 PM, Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote: > The current Branch History Rolling Buffer (BHRB) code does > not check for any privilege levels before updating the data > from BHRB. This leaks kernel addresses to userspace even when > profiling only with userspace privileges. Add proper checks > to prevent it. > > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > --- > arch/powerpc/perf/core-book3s.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index f89bbd54ecec..337db5831749 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -457,6 +457,10 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw) > /* invalid entry */ > continue; > > + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && > + is_kernel_addr(addr)) > + continue; > + Looks good to me. The scope of the leaks concern is KASLR related or something else (figuring out what's in the cache?) Acked-by: Balbir Singh <bsingharora@gmail.com> Balbir Singh. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer 2018-03-05 6:16 ` [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Balbir Singh @ 2018-03-07 4:54 ` Madhavan Srinivasan 0 siblings, 0 replies; 6+ messages in thread From: Madhavan Srinivasan @ 2018-03-07 4:54 UTC (permalink / raw) To: Balbir Singh Cc: Michael Ellerman, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT) On Monday 05 March 2018 11:46 AM, Balbir Singh wrote: > On Sun, Mar 4, 2018 at 10:55 PM, Madhavan Srinivasan > <maddy@linux.vnet.ibm.com> wrote: >> The current Branch History Rolling Buffer (BHRB) code does >> not check for any privilege levels before updating the data >> from BHRB. This leaks kernel addresses to userspace even when >> profiling only with userspace privileges. Add proper checks >> to prevent it. >> >> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> --- >> arch/powerpc/perf/core-book3s.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c >> index f89bbd54ecec..337db5831749 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -457,6 +457,10 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw) >> /* invalid entry */ >> continue; >> >> + if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) && >> + is_kernel_addr(addr)) >> + continue; >> + > > Looks good to me. The scope of the leaks concern is KASLR related or > something else (figuring out what's in the cache?) I did not look at it closely. But will get the information. Thanks for the review Maddy > > Acked-by: Balbir Singh <bsingharora@gmail.com> > > Balbir Singh. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-07 4:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-04 11:55 [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Madhavan Srinivasan 2018-03-04 11:55 ` [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR Madhavan Srinivasan 2018-03-05 8:21 ` Naveen N. Rao 2018-03-07 4:53 ` Madhavan Srinivasan 2018-03-05 6:16 ` [PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer Balbir Singh 2018-03-07 4:54 ` Madhavan Srinivasan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).