* [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 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 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-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).