linux-kernel - Re: [PATCH] drivers/perf: Fix some null pointer dereference issues in thunderx2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9586be60-35f9-401f-b473-b16e86d13d07@kylinos.cn>
Date:   Thu, 14 Dec 2023 10:57:38 +0800
From:   Kunwu Chan <chentao@...inos.cn>
To:     Will Deacon <will@...nel.org>
Cc:     mark.rutland@....com, Ganapatrao.Kulkarni@...ium.com,
        suzuki.poulose@....com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Kunwu Chan <kunwu.chan@...mail.com>
Subject: Re: [PATCH] drivers/perf: Fix some null pointer dereference issues in
 thunderx2_pmu.c

Thanks for your reply.

After read tx2_uncore_pmu_register again.
 From the defination: 'char *name = tx2_pmu->name;',
we could know 'tx2_pmu->pmu.name' equals 'tx2_pmu->name'

The difference is that a new memory space is allocated for 
'tx2_pmu->pmu.name'.

If 'tx2_pmu->pmu.name' is always the same as 'tx2_pmu->name', whether we 
should use 'tx2_pmu->pmu.name =  tx2_pmu->name;'
to replace the 'devm_kasprintf'.

I'm not sure it's appropriate to do that.

Thanks again.


On 2023/12/12 17:25, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 05:03:47PM +0800, Kunwu Chan wrote:
>> devm_kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
>>
>> Fixes: 69c32972d593 ("drivers/perf: Add Cavium ThunderX2 SoC UNCORE PMU driver")
>> Cc: Kunwu Chan <kunwu.chan@...mail.com>
>> Signed-off-by: Kunwu Chan <chentao@...inos.cn>
>> ---
>>   drivers/perf/thunderx2_pmu.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
>> index 1edb9c03704f..07edb174a0d7 100644
>> --- a/drivers/perf/thunderx2_pmu.c
>> +++ b/drivers/perf/thunderx2_pmu.c
>> @@ -742,6 +742,8 @@ static int tx2_uncore_pmu_register(
>>   
>>   	tx2_pmu->pmu.name = devm_kasprintf(dev, GFP_KERNEL,
>>   			"%s", name);
>> +	if (!tx2_pmu->pmu.name)
>> +		return -ENOMEM;
>>   
>>   	return perf_pmu_register(&tx2_pmu->pmu, tx2_pmu->pmu.name, -1);
> 
> AFAICT, perf_pmu_register() will WARN and return NULL, so I'm not sure what
> we gain from the additional check.
> 
>>   }
>> @@ -881,6 +883,11 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>>   		return NULL;
>>   	}
>>   
>> +	if (!tx2_pmu->name) {
>> +		dev_err(dev, "PMU type %d: Fail to allocate memory\n", type);
>> +		devm_kfree(dev, tx2_pmu);
>> +		return NULL;
>> +	}
> 
> In the _highly_ unlikely even that devm_kasprintf() failed to allocate,
> shouldn't we get a splat from the allocator? I don't think it's useful
> to print another message.
> 
> Will