Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1ac1e478-8f31-da78-0df7-d34305544aa6@rasmusvillemoes.dk>
Date:   Fri, 18 Feb 2022 09:46:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation
 on renaming race
Content-Language: en-US
To:     "Pighin, Anthony (Nokia - CA/Ottawa)" <anthony.pighin@nokia.com>,
        "thomas.lendacky@amd.com" <thomas.lendacky@amd.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <BN0PR08MB6951BF9664D8BB97AB4C753183369@BN0PR08MB6951.namprd08.prod.outlook.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <BN0PR08MB6951BF9664D8BB97AB4C753183369@BN0PR08MB6951.namprd08.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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