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

Date:   Fri, 18 Feb 2022 12:42:41 +0000
From:   "Pighin, Anthony (Nokia - CA/Ottawa)" <anthony.pighin@...ia.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        "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

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@...musvillemoes.dk> 
Sent: Friday, February 18, 2022 3:46 AM
To: Pighin, Anthony (Nokia - CA/Ottawa) <anthony.pighin@...ia.com>; thomas.lendacky@....com
Cc: 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