linux-hardening - Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy

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

Date: Wed, 20 Mar 2024 23:48:52 +0100
From: Helge Deller <deller@....de>
To: Justin Stitt <justinstitt@...gle.com>
Cc: linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
 Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with
 strscpy

On 3/20/24 23:35, Justin Stitt wrote:
> Hi,
>
> On Wed, Mar 20, 2024 at 12:56 AM Helge Deller <deller@....de> wrote:
>>
>> On 3/19/24 00:46, Justin Stitt wrote:
>>> strncpy() is deprecated for use on NUL-terminated destination strings
>>> [1] and as such we should prefer more robust and less ambiguous string
>>> interfaces.
>>>
>>> Let's use the new 2-argument strscpy() which guarantees NUL-termination
>>> on the destination buffer while also simplifying the syntax. Note that
>>> strscpy() will not NUL-pad the destination buffer like strncpy() does.
>>>
>>> However, the NUL-padding behavior of strncpy() is not required since
>>> fbdev is already NUL-allocated from au1200fb_drv_probe() ->
>>> frameuffer_alloc(), rendering any additional NUL-padding redundant.
>>> |     p = kzalloc(fb_info_size + size, GFP_KERNEL);
>>>
>>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
>>> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
>>> Link: https://github.com/KSPP/linux/issues/90
>>> Cc: linux-hardening@...r.kernel.org
>>> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
>>> ---
>>> Note: build-tested only.
>>>
>>> Found with: $ rg "strncpy\("
>>> ---
>>>    drivers/video/fbdev/au1200fb.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
>>> index 6f20efc663d7..e718fea63662 100644
>>> --- a/drivers/video/fbdev/au1200fb.c
>>> +++ b/drivers/video/fbdev/au1200fb.c
>>> @@ -1557,7 +1557,7 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
>>>                return ret;
>>>        }
>>>
>>> -     strncpy(fbi->fix.id, "AU1200", sizeof(fbi->fix.id));
>>> +     strscpy(fbi->fix.id, "AU1200");
>>
>> I wonder if you really build-tested this, as this driver is for the mips architecture...
>> And I don't see a strscpy() function which takes just 2 arguments.
>> But I might be wrong....
>
> I did build successfully :thumbs_up:
>
> Commit e6584c3964f2f ("string: Allow 2-argument strscpy()") introduced
> this new strscpy() form; it is present in string.h on Linus' tree.

Interesting patch.
Might give compile problems if patches like yours gets automatically
picked up to stable series as long as Kees patch hasn't been backported yet...
Anyway, thanks for the pointer!
I'll apply your patch in the next round for fbdev.

Helge