linux-kernel - Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9c59d1b1-a483-49d9-b57a-c86e3e020234@linaro.org>
Date: Fri, 4 Jul 2025 12:10:53 +0100
From: James Clark <james.clark@...aro.org>
To: David Laight <david.laight.linux@...il.com>,
 Usman Akinyemi <usmanakinyemi202@...il.com>
Cc: peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
 namhyung@...nel.org, mark.rutland@....com,
 alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
 adrian.hunter@...el.com, kan.liang@...ux.intel.com,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor
 string



On 04/07/2025 10:20 am, David Laight wrote:
> On Thu, 19 Jun 2025 03:28:43 +0530
> Usman Akinyemi <usmanakinyemi202@...il.com> wrote:
> 
>> strncpy() is unsafe for fixed-size binary data as
>> it may not NUL-terminate and is deprecated for such

But memcpy doesn't null terminate after the 4 chars either so I don't 
think that's a good justification. Surely you don't want null 
termination, because char *vendor is supposed to be a single string 
without extra nulls in the middle. It specifically adds a null at the 
end of the function.

>> usage. Since we're copying raw CPUID register values,
>> memcpy() is the correct and safe choice.
>>

There should be a fixes: tag here if it actually fixes something. But in 
this use case strncpy seems to behave identically to memcpy so I don't 
think we should change it. Except maybe if b,c,d have NULLs in them then 
strncpy will give you uninitialized parts where memcpy won't. But that's 
not mentioned in the commit message and presumably it doesn't happen?

>> Signed-off-by: Usman Akinyemi <usmanakinyemi202@...il.com>
>> ---
>>   tools/perf/arch/x86/util/header.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
>> index 412977f8aa83..43ba55627817 100644
>> --- a/tools/perf/arch/x86/util/header.c
>> +++ b/tools/perf/arch/x86/util/header.c
>> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
>>   	unsigned int b, c, d;
>>   
>>   	cpuid(0, 0, lvl, &b, &c, &d);
>> -	strncpy(&vendor[0], (char *)(&b), 4);
>> -	strncpy(&vendor[4], (char *)(&d), 4);
>> -	strncpy(&vendor[8], (char *)(&c), 4);
>> +	memcpy(&vendor[0], (char *)(&b), 4);
>> +	memcpy(&vendor[4], (char *)(&d), 4);
>> +	memcpy(&vendor[8], (char *)(&c), 4);
> 
> Why not:
> 	cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));
> 
> 
>>   	vendor[12] = '\0';
>>   }
>>   
>