Re: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation on renaming race

Re: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation on renaming race

[Rasmus Villemoes]

On 17/02/2022 15.39, Pighin, Anthony (Nokia - CA/Ottawa) wrote:
> PROBLEM: Commit 8e2a2bfdb86ecb2421e3dd18d0fbbb42f2d943ad is being
> triggered during debugfs entry creation in
> ./drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
> 
>  
> 
> Seen in 5.4.153, 5.15.22, and 5.17-rc3 (ie. this bug is still present).
> 


Well, it's not really 8e2a2bfdb that is buggy, but the caller of
kasprintf() that doesn't prevent a %s argument from changing midway
through the call. In fact this simply shows that 8e2a2bfdb was justified.

In this case, since IFNAMSIZ is just 16, it would probably be simplest
and easiest just to avoid kasprintf() and use a small stack buffer

- char *buf;
+ char buf[32];

- buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
- if (!buf)
-   return;
+ snprintf(buf, sizeof(buf), "amd-xgbe-%s", pdata->netdev->name);

- kfree(buf);

It's still possible to get garbage in the output (a mix of the old and
new name), of course.

But IIRC correctly, updating the netdev->name is done carefully in a way
that there's always a nul-terminator, so snprintf() should not
accidentally walk off the end of netdev->name [look at string() in
vsprintf.c, it has been carefully rewritten to not walk the string
twice]. And if not, there's really nothing snprintf() or kasprintf()
could do about passing a racy netdev->name .

And while in there, the use of kasprintf() in xgbe_common_read() might
as well be avoided, a 16 byte stack buffer is plenty, and removes the
need for the 'if (!buf)' check and multiple kfree(buf) on the various
return paths.

Rasmus

RE: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation on renaming race

[Pighin, Anthony (Nokia - CA/Ottawa)]

Hello Rasmus,

Thank you for your reply.

Perhaps my wording was off. You are correct that 8e2a2bfdb is not at fault. The commit log says "let's see if it ever triggers", and that is what I was trying to flag.

Anthony

-----Original Message-----
From: Rasmus Villemoes <linux@rasmusvillemoes.dk> 
Sent: Friday, February 18, 2022 3:46 AM
To: Pighin, Anthony (Nokia - CA/Ottawa) <anthony.pighin@nokia.com>; thomas.lendacky@amd.com
Cc: linux-kernel@vger.kernel.org
Subject: Re: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation on renaming race

On 17/02/2022 15.39, Pighin, Anthony (Nokia - CA/Ottawa) wrote:
> PROBLEM: Commit 8e2a2bfdb86ecb2421e3dd18d0fbbb42f2d943ad is being 
> triggered during debugfs entry creation in 
> ./drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
> 
>  
> 
> Seen in 5.4.153, 5.15.22, and 5.17-rc3 (ie. this bug is still present).
> 


Well, it's not really 8e2a2bfdb that is buggy, but the caller of
kasprintf() that doesn't prevent a %s argument from changing midway through the call. In fact this simply shows that 8e2a2bfdb was justified.

In this case, since IFNAMSIZ is just 16, it would probably be simplest and easiest just to avoid kasprintf() and use a small stack buffer

- char *buf;
+ char buf[32];

- buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
- if (!buf)
-   return;
+ snprintf(buf, sizeof(buf), "amd-xgbe-%s", pdata->netdev->name);

- kfree(buf);

It's still possible to get garbage in the output (a mix of the old and new name), of course.

But IIRC correctly, updating the netdev->name is done carefully in a way that there's always a nul-terminator, so snprintf() should not accidentally walk off the end of netdev->name [look at string() in vsprintf.c, it has been carefully rewritten to not walk the string twice]. And if not, there's really nothing snprintf() or kasprintf() could do about passing a racy netdev->name .

And while in there, the use of kasprintf() in xgbe_common_read() might as well be avoided, a 16 byte stack buffer is plenty, and removes the need for the 'if (!buf)' check and multiple kfree(buf) on the various return paths.

Rasmus