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

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

Message-ID: <1ac1e478-8f31-da78-0df7-d34305544aa6@rasmusvillemoes.dk>
Date:   Fri, 18 Feb 2022 09:46:11 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     "Pighin, Anthony (Nokia - CA/Ottawa)" <anthony.pighin@...ia.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.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