Re: [PATCH] powerpc/papr_scm: Fix buffer overflow issue with CONFIG_FORTIFY_SOURCE

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Kajol,

Thanks for the patch. Minor review comment below:

Kajol Jain <kjain@linux.ibm.com> writes:

> With CONFIG_FORTIFY_SOURCE enabled, string functions will also perform
> dynamic checks for string size which can panic the kernel,
> like incase of overflow detection.
>
> In papr_scm, papr_scm_pmu_check_events function uses stat->stat_id
> with string operations, to populate the nvdimm_events_map array.
> Since stat_id variable is not NULL terminated, the kernel panics
> with CONFIG_FORTIFY_SOURCE enabled at boot time.
>
> Below are the logs of kernel panic:
>
> [    0.090221][    T1] detected buffer overflow in __fortify_strlen
> [    0.090241][    T1] ------------[ cut here ]------------
> [    0.090246][    T1] kernel BUG at lib/string_helpers.c:980!
> [    0.090253][    T1] Oops: Exception in kernel mode, sig: 5 [#1]
> ........
> [    0.090375][    T1] NIP [c00000000077dad0] fortify_panic+0x28/0x38
> [    0.090382][    T1] LR [c00000000077dacc] fortify_panic+0x24/0x38
> [    0.090387][    T1] Call Trace:
> [    0.090390][    T1] [c0000022d77836e0] [c00000000077dacc] fortify_panic+0x24/0x38 (unreliable)
> [    9.297707] [    T1] [c00800000deb2660] papr_scm_pmu_check_events.constprop.0+0x118/0x220 [papr_scm]
> [    9.297721] [    T1] [c00800000deb2cb0] papr_scm_probe+0x288/0x62c [papr_scm]
> [    9.297732] [    T1] [c0000000009b46a8] platform_probe+0x98/0x150
>
> Fix this issue by using kmemdup_nul function to copy the content of
> stat->stat_id directly to the nvdimm_events_map array.
>
> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index f58728d5f10d..39962c905542 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -462,7 +462,6 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
>  {
>  	struct papr_scm_perf_stat *stat;
>  	struct papr_scm_perf_stats *stats;
> -	char *statid;
>  	int index, rc, count;
>  	u32 available_events;
>  
> @@ -493,14 +492,12 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
>  
>  	for (index = 0, stat = stats->scm_statistic, count = 0;
>  		     index < available_events; index++, ++stat) {
> -		statid = kzalloc(strlen(stat->stat_id) + 1, GFP_KERNEL);
> -		if (!statid) {
> +		p->nvdimm_events_map[count] = kmemdup_nul(stat->stat_id,
>  	8, GFP_KERNEL);
s/8/sizeof(stat->stat_id)/

> +		if (!p->nvdimm_events_map[count]) {
>  			rc = -ENOMEM;
>  			goto out_nvdimm_events_map;
>  		}
>  
> -		strcpy(statid, stat->stat_id);
> -		p->nvdimm_events_map[count] = statid;
>  		count++;
>  	}
>  	p->nvdimm_events_map[count] = NULL;
> -- 
> 2.31.1
>

-- 
Cheers
~ Vaibhav