netdev - Re: [PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name

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]

Date:   Fri, 18 Feb 2022 15:46:28 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     David Laight <David.Laight@...LAB.COM>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     David Miller <davem@...emloft.net>,
        Shyam-sundar S-k <Shyam-sundar.S-k@....com>,
        Anthony Pighin <anthony.pighin@...ia.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf()
 for debugfs name

On 2/18/22 15:01, David Laight wrote:
> From: Tom Lendacky
>> Sent: 18 February 2022 20:05
>>
>> It was reported that using kasprintf() produced a kernel warning as the
>> network interface name was being changed by udev rules at the same time
>> that the debugfs entry for the device was being created.
> 
> What was the error?
> I'm guessing the length changed and that made kvasprintf() unhappy??
> 
> ...
>> -	buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
>> -	if (!buf)
>> +	ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX,
>> +		       pdata->netdev->name);
> 
> You can do:
> 	snprintf(buf, sizeof buf, XGBE_DIR_PREFIX "%s", pdata->netdev->name)
> 

Thought about that, just decided on the double %s, though.

>> +	if (ret >= sizeof(buf))
>>   		return;
> 
> Unlike kasnprintf() where kmalloc() can fail, the simple snprintf()
> can't really overrun unless pdata->netdev->name isn't '\0' terminated.
> Even if it being changed while you look at it that shouldn't happen.

It's a safety check, it doesn't hurt anything.

> 
> 
> Don't you need to synchronise this anyway?
> 
> If the debugfs create and rename can happen at the same time then
> the rename can be requested before the create and you get the wrong
> name.

I thought about introducing a mutex or semaphore, but thought it was 
overkill just for debugfs support and this small window. But if folks 
think it's really needed, it can be added.

Thanks,
Tom

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>