mbox series

[PATCHv2,0/4] coresight: etf/etb10/etr: Fix NULL pointer dereference crashes

Message ID cover.1603363729.git.saiprakash.ranjan@codeaurora.org
Headers show
Series coresight: etf/etb10/etr: Fix NULL pointer dereference crashes | expand

Message

Sai Prakash Ranjan Oct. 22, 2020, 10:57 a.m. UTC
There was a report of NULL pointer dereference in ETF enable
path for perf CS mode with PID monitoring. It is almost 100%
reproducible when the process to monitor is something very
active such as chrome and with ETF as the sink and not ETR.
Currently in a bid to find the pid, the owner is dereferenced
via task_pid_nr() call in tmc_enable_etf_sink_perf() and with
owner being NULL, we get a NULL pointer dereference.

Looking at the ETR and other places in the kernel, ETF and the
ETB are the only places trying to dereference the task(owner)
in tmc_enable_etf_sink_perf() which is also called from the
sched_in path as in the call trace. Owner(task) is NULL even
in the case of ETR in tmc_enable_etr_sink_perf(), but since we
cache the PID in alloc_buffer() callback and it is done as part
of etm_setup_aux() when allocating buffer for ETR sink, we never
dereference this NULL pointer and we are safe. So lets do the
same thing with ETF and ETB and cache the PID to which the
cs_buffer belongs in alloc_buffer() callback for ETF and ETB as
done for ETR. This will also remove the unnecessary function calls
(task_pid_nr()) in tmc_enable_etr_sink_perf() and etb_enable_perf().

In addition to this, add a check to validate event->owner before
dereferencing it in ETR, ETB and ETF to avoid any possible NULL
pointer dereference crashes in their corresponding alloc_buffer
callbacks and check for kernel events as well.

Easily reproducible running below:

 perf record -e cs_etm/@tmc_etf0/ -N -p <pid>

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000548
Mem abort info:
  ESR = 0x96000006
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000006
  CM = 0, WnR = 0
<snip>...
Call trace:
 tmc_enable_etf_sink+0xe4/0x280
 coresight_enable_path+0x168/0x1fc
 etm_event_start+0x8c/0xf8
 etm_event_add+0x38/0x54
 event_sched_in+0x194/0x2ac
 group_sched_in+0x54/0x12c
 flexible_sched_in+0xd8/0x120
 visit_groups_merge+0x100/0x16c
 ctx_flexible_sched_in+0x50/0x74
 ctx_sched_in+0xa4/0xa8
 perf_event_sched_in+0x60/0x6c
 perf_event_context_sched_in+0x98/0xe0
 __perf_event_task_sched_in+0x5c/0xd8
 finish_task_switch+0x184/0x1cc
 schedule_tail+0x20/0xec
 ret_from_fork+0x4/0x18

Sai Prakash Ranjan (4):
  perf/core: Export is_kernel_event()
  coresight: tmc-etf: Fix NULL ptr dereference in
    tmc_enable_etf_sink_perf()
  coresight: etb10: Fix possible NULL ptr dereference in
    etb_enable_perf()
  coresight: tmc-etr: Fix possible NULL ptr dereference in
    get_perf_etr_buf_cpu_wide()

 drivers/hwtracing/coresight/coresight-etb10.c   | 8 +++++++-
 drivers/hwtracing/coresight/coresight-priv.h    | 2 ++
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 8 +++++++-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 6 +++++-
 include/linux/perf_event.h                      | 2 ++
 kernel/events/core.c                            | 3 ++-
 6 files changed, 25 insertions(+), 4 deletions(-)


base-commit: f4cb5e9daedf56671badc93ac7f364043aa33886

Comments

Sai Prakash Ranjan Oct. 22, 2020, 11:10 a.m. UTC | #1
On 2020-10-22 16:27, Sai Prakash Ranjan wrote:
> There was a report of NULL pointer dereference in ETF enable

> path for perf CS mode with PID monitoring. It is almost 100%

> reproducible when the process to monitor is something very

> active such as chrome and with ETF as the sink and not ETR.

> Currently in a bid to find the pid, the owner is dereferenced

> via task_pid_nr() call in tmc_enable_etf_sink_perf() and with

> owner being NULL, we get a NULL pointer dereference.

> 

> Looking at the ETR and other places in the kernel, ETF and the

> ETB are the only places trying to dereference the task(owner)

> in tmc_enable_etf_sink_perf() which is also called from the

> sched_in path as in the call trace. Owner(task) is NULL even

> in the case of ETR in tmc_enable_etr_sink_perf(), but since we

> cache the PID in alloc_buffer() callback and it is done as part

> of etm_setup_aux() when allocating buffer for ETR sink, we never

> dereference this NULL pointer and we are safe. So lets do the

> same thing with ETF and ETB and cache the PID to which the

> cs_buffer belongs in alloc_buffer() callback for ETF and ETB as

> done for ETR. This will also remove the unnecessary function calls

> (task_pid_nr()) in tmc_enable_etr_sink_perf() and etb_enable_perf().

> 

> In addition to this, add a check to validate event->owner before

> dereferencing it in ETR, ETB and ETF to avoid any possible NULL

> pointer dereference crashes in their corresponding alloc_buffer

> callbacks and check for kernel events as well.

> 

> Easily reproducible running below:

> 

>  perf record -e cs_etm/@tmc_etf0/ -N -p <pid>

> 

> Unable to handle kernel NULL pointer dereference at virtual address

> 0000000000000548

> Mem abort info:

>   ESR = 0x96000006

>   EC = 0x25: DABT (current EL), IL = 32 bits

>   SET = 0, FnV = 0

>   EA = 0, S1PTW = 0

> Data abort info:

>   ISV = 0, ISS = 0x00000006

>   CM = 0, WnR = 0

> <snip>...

> Call trace:

>  tmc_enable_etf_sink+0xe4/0x280

>  coresight_enable_path+0x168/0x1fc

>  etm_event_start+0x8c/0xf8

>  etm_event_add+0x38/0x54

>  event_sched_in+0x194/0x2ac

>  group_sched_in+0x54/0x12c

>  flexible_sched_in+0xd8/0x120

>  visit_groups_merge+0x100/0x16c

>  ctx_flexible_sched_in+0x50/0x74

>  ctx_sched_in+0xa4/0xa8

>  perf_event_sched_in+0x60/0x6c

>  perf_event_context_sched_in+0x98/0xe0

>  __perf_event_task_sched_in+0x5c/0xd8

>  finish_task_switch+0x184/0x1cc

>  schedule_tail+0x20/0xec

>  ret_from_fork+0x4/0x18

> 

> Sai Prakash Ranjan (4):

>   perf/core: Export is_kernel_event()

>   coresight: tmc-etf: Fix NULL ptr dereference in

>     tmc_enable_etf_sink_perf()

>   coresight: etb10: Fix possible NULL ptr dereference in

>     etb_enable_perf()

>   coresight: tmc-etr: Fix possible NULL ptr dereference in

>     get_perf_etr_buf_cpu_wide()

> 

>  drivers/hwtracing/coresight/coresight-etb10.c   | 8 +++++++-

>  drivers/hwtracing/coresight/coresight-priv.h    | 2 ++

>  drivers/hwtracing/coresight/coresight-tmc-etf.c | 8 +++++++-

>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 6 +++++-

>  include/linux/perf_event.h                      | 2 ++

>  kernel/events/core.c                            | 3 ++-

>  6 files changed, 25 insertions(+), 4 deletions(-)

> 

> 

> base-commit: f4cb5e9daedf56671badc93ac7f364043aa33886


Please ignore this series, I will need to resend.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation
Sai Prakash Ranjan Oct. 22, 2020, 11:23 a.m. UTC | #2
On 2020-10-22 16:40, Sai Prakash Ranjan wrote:
> On 2020-10-22 16:27, Sai Prakash Ranjan wrote:
>> There was a report of NULL pointer dereference in ETF enable
>> path for perf CS mode with PID monitoring. It is almost 100%
>> reproducible when the process to monitor is something very
>> active such as chrome and with ETF as the sink and not ETR.
>> Currently in a bid to find the pid, the owner is dereferenced
>> via task_pid_nr() call in tmc_enable_etf_sink_perf() and with
>> owner being NULL, we get a NULL pointer dereference.
>> 
>> Looking at the ETR and other places in the kernel, ETF and the
>> ETB are the only places trying to dereference the task(owner)
>> in tmc_enable_etf_sink_perf() which is also called from the
>> sched_in path as in the call trace. Owner(task) is NULL even
>> in the case of ETR in tmc_enable_etr_sink_perf(), but since we
>> cache the PID in alloc_buffer() callback and it is done as part
>> of etm_setup_aux() when allocating buffer for ETR sink, we never
>> dereference this NULL pointer and we are safe. So lets do the
>> same thing with ETF and ETB and cache the PID to which the
>> cs_buffer belongs in alloc_buffer() callback for ETF and ETB as
>> done for ETR. This will also remove the unnecessary function calls
>> (task_pid_nr()) in tmc_enable_etr_sink_perf() and etb_enable_perf().
>> 
>> In addition to this, add a check to validate event->owner before
>> dereferencing it in ETR, ETB and ETF to avoid any possible NULL
>> pointer dereference crashes in their corresponding alloc_buffer
>> callbacks and check for kernel events as well.
>> 
>> Easily reproducible running below:
>> 
>>  perf record -e cs_etm/@tmc_etf0/ -N -p <pid>
>> 
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 0000000000000548
>> Mem abort info:
>>   ESR = 0x96000006
>>   EC = 0x25: DABT (current EL), IL = 32 bits
>>   SET = 0, FnV = 0
>>   EA = 0, S1PTW = 0
>> Data abort info:
>>   ISV = 0, ISS = 0x00000006
>>   CM = 0, WnR = 0
>> <snip>...
>> Call trace:
>>  tmc_enable_etf_sink+0xe4/0x280
>>  coresight_enable_path+0x168/0x1fc
>>  etm_event_start+0x8c/0xf8
>>  etm_event_add+0x38/0x54
>>  event_sched_in+0x194/0x2ac
>>  group_sched_in+0x54/0x12c
>>  flexible_sched_in+0xd8/0x120
>>  visit_groups_merge+0x100/0x16c
>>  ctx_flexible_sched_in+0x50/0x74
>>  ctx_sched_in+0xa4/0xa8
>>  perf_event_sched_in+0x60/0x6c
>>  perf_event_context_sched_in+0x98/0xe0
>>  __perf_event_task_sched_in+0x5c/0xd8
>>  finish_task_switch+0x184/0x1cc
>>  schedule_tail+0x20/0xec
>>  ret_from_fork+0x4/0x18
>> 
>> Sai Prakash Ranjan (4):
>>   perf/core: Export is_kernel_event()
>>   coresight: tmc-etf: Fix NULL ptr dereference in
>>     tmc_enable_etf_sink_perf()
>>   coresight: etb10: Fix possible NULL ptr dereference in
>>     etb_enable_perf()
>>   coresight: tmc-etr: Fix possible NULL ptr dereference in
>>     get_perf_etr_buf_cpu_wide()
>> 
>>  drivers/hwtracing/coresight/coresight-etb10.c   | 8 +++++++-
>>  drivers/hwtracing/coresight/coresight-priv.h    | 2 ++
>>  drivers/hwtracing/coresight/coresight-tmc-etf.c | 8 +++++++-
>>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 6 +++++-
>>  include/linux/perf_event.h                      | 2 ++
>>  kernel/events/core.c                            | 3 ++-
>>  6 files changed, 25 insertions(+), 4 deletions(-)
>> 
>> 
>> base-commit: f4cb5e9daedf56671badc93ac7f364043aa33886
> 
> Please ignore this series, I will need to resend.
> 

Please ignore my previous ignore request and let me
know if exporting is_kernel_event() is ok.

Thanks,
Sai