* [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
@ 2024-03-18 23:46 Justin Stitt
2024-03-20 2:44 ` Kees Cook
2024-03-20 7:56 ` Helge Deller
0 siblings, 2 replies; 7+ messages in thread
From: Justin Stitt @ 2024-03-18 23:46 UTC (permalink / raw)
To: Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel, linux-hardening,
Justin Stitt
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@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.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");
fbi->fix.smem_start = fbdev->fb_phys;
fbi->fix.smem_len = fbdev->fb_len;
fbi->fix.type = FB_TYPE_PACKED_PIXELS;
---
base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400
change-id: 20240318-strncpy-drivers-video-fbdev-au1200fb-c-7bc337998096
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-03-18 23:46 [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy Justin Stitt
@ 2024-03-20 2:44 ` Kees Cook
2024-03-20 7:56 ` Helge Deller
1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2024-03-20 2:44 UTC (permalink / raw)
To: Justin Stitt
Cc: Helge Deller, linux-fbdev, dri-devel, linux-kernel,
linux-hardening
On Mon, Mar 18, 2024 at 11:46:33PM +0000, 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@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
Yup, looks correct.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-03-18 23:46 [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy Justin Stitt
2024-03-20 2:44 ` Kees Cook
@ 2024-03-20 7:56 ` Helge Deller
2024-03-20 22:35 ` Justin Stitt
1 sibling, 1 reply; 7+ messages in thread
From: Helge Deller @ 2024-03-20 7:56 UTC (permalink / raw)
To: Justin Stitt
Cc: linux-fbdev, dri-devel, linux-kernel, linux-hardening, Kees Cook
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@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.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....
Helge
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-03-20 7:56 ` Helge Deller
@ 2024-03-20 22:35 ` Justin Stitt
2024-03-20 22:48 ` Helge Deller
0 siblings, 1 reply; 7+ messages in thread
From: Justin Stitt @ 2024-03-20 22:35 UTC (permalink / raw)
To: Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel, linux-hardening, Kees Cook
Hi,
On Wed, Mar 20, 2024 at 12:56 AM Helge Deller <deller@gmx.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@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.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.
>
> Helge
Thanks
Justin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-03-20 22:35 ` Justin Stitt
@ 2024-03-20 22:48 ` Helge Deller
2024-04-24 23:49 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Helge Deller @ 2024-03-20 22:48 UTC (permalink / raw)
To: Justin Stitt
Cc: linux-fbdev, dri-devel, linux-kernel, linux-hardening, Kees Cook
On 3/20/24 23:35, Justin Stitt wrote:
> Hi,
>
> On Wed, Mar 20, 2024 at 12:56 AM Helge Deller <deller@gmx.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@vger.kernel.org
>>> Signed-off-by: Justin Stitt <justinstitt@google.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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-03-20 22:48 ` Helge Deller
@ 2024-04-24 23:49 ` Kees Cook
2024-04-25 10:00 ` Helge Deller
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2024-04-24 23:49 UTC (permalink / raw)
To: Helge Deller
Cc: Justin Stitt, linux-fbdev, dri-devel, linux-kernel,
linux-hardening
On Wed, Mar 20, 2024 at 11:48:52PM +0100, Helge Deller wrote:
> On 3/20/24 23:35, Justin Stitt wrote:
> > Hi,
> >
> > On Wed, Mar 20, 2024 at 12:56 AM Helge Deller <deller@gmx.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@vger.kernel.org
> > > > Signed-off-by: Justin Stitt <justinstitt@google.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.
Hi! I haven't seen this show up in -next yet. Have you had a chance to
pick it up?
There are also these too:
https://lore.kernel.org/all/20240320-strncpy-drivers-video-fbdev-fsl-diu-fb-c-v1-1-3cd3c012fa8c@google.com/
https://patchwork.kernel.org/project/linux-hardening/patch/20240320-strncpy-drivers-video-fbdev-uvesafb-c-v1-1-fd6af3766c80@google.com/
https://patchwork.kernel.org/project/linux-hardening/patch/20240320-strncpy-drivers-video-hdmi-c-v1-1-f9a08168cdaf@google.com/
I can toss all of these into the hardening tree if that makes it easier
for you?
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy
2024-04-24 23:49 ` Kees Cook
@ 2024-04-25 10:00 ` Helge Deller
0 siblings, 0 replies; 7+ messages in thread
From: Helge Deller @ 2024-04-25 10:00 UTC (permalink / raw)
To: Kees Cook
Cc: Justin Stitt, linux-fbdev, dri-devel, linux-kernel,
linux-hardening
On 4/25/24 01:49, Kees Cook wrote:
> On Wed, Mar 20, 2024 at 11:48:52PM +0100, Helge Deller wrote:
>> On 3/20/24 23:35, Justin Stitt wrote:
>>> Hi,
>>>
>>> On Wed, Mar 20, 2024 at 12:56 AM Helge Deller <deller@gmx.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@vger.kernel.org
>>>>> Signed-off-by: Justin Stitt <justinstitt@google.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.
>
> Hi! I haven't seen this show up in -next yet. Have you had a chance to
> pick it up?
>
> There are also these too:
>
> https://lore.kernel.org/all/20240320-strncpy-drivers-video-fbdev-fsl-diu-fb-c-v1-1-3cd3c012fa8c@google.com/
> https://patchwork.kernel.org/project/linux-hardening/patch/20240320-strncpy-drivers-video-fbdev-uvesafb-c-v1-1-fd6af3766c80@google.com/
> https://patchwork.kernel.org/project/linux-hardening/patch/20240320-strncpy-drivers-video-hdmi-c-v1-1-f9a08168cdaf@google.com/
All 4 patches picked up into fbdev for-next git tree now.
Thanks!
Helge
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-25 10:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-18 23:46 [PATCH] video: fbdev: au1200fb: replace deprecated strncpy with strscpy Justin Stitt
2024-03-20 2:44 ` Kees Cook
2024-03-20 7:56 ` Helge Deller
2024-03-20 22:35 ` Justin Stitt
2024-03-20 22:48 ` Helge Deller
2024-04-24 23:49 ` Kees Cook
2024-04-25 10:00 ` Helge Deller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).