linux-um.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] um: replace deprecated strncpy with strscpy
@ 2025-06-07 21:30 Brahmajit Das
  2025-06-10  4:15 ` Kees Cook
  0 siblings, 1 reply; 3+ messages in thread
From: Brahmajit Das @ 2025-06-07 21:30 UTC (permalink / raw)
  To: linux-hardening
  Cc: kees, justinstitt, richard, anton.ivanov, johannes, linux-um

strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

This modification is mainly due to the concerns on
https://github.com/KSPP/linux/issues/336, where it was mentioned that
strncpy_from_user is confusingly named as it does not NUL-pad the
destination, but it does NOT guarantee NUL-termination.
With this approach/patch we can always ensure that the dst buffer is NUL
terminated.

My initial guess was to create a separate function, something like
strscpy_from_user that would use strscpy_chunk_from_user which would
inturn use strscpy instead of the strncpy (like in this patch). But I'm
new quite unsure about adding new functions to kernel code. I'm open to
other ideas and/or approach as well, since I'm quit sure there might be
a better way to handle this.

[1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings

Signed-off-by: Brahmajit Das <listout@listout.xyz>
---
 arch/um/kernel/skas/uaccess.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
index 198269e384c4..c00ef385591c 100644
--- a/arch/um/kernel/skas/uaccess.c
+++ b/arch/um/kernel/skas/uaccess.c
@@ -170,7 +170,7 @@ static int strncpy_chunk_from_user(unsigned long from, int len, void *arg)
 	char **to_ptr = arg, *to = *to_ptr;
 	int n;
 
-	strncpy(to, (void *) from, len);
+	strscpy(to, (void *) from, len);
 	n = strnlen(to, len);
 	*to_ptr += n;
 
-- 
2.49.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [RFC PATCH] um: replace deprecated strncpy with strscpy
  2025-06-07 21:30 [RFC PATCH] um: replace deprecated strncpy with strscpy Brahmajit Das
@ 2025-06-10  4:15 ` Kees Cook
  2025-06-12 16:09   ` Brahmajit Das
  0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2025-06-10  4:15 UTC (permalink / raw)
  To: Brahmajit Das
  Cc: linux-hardening, justinstitt, richard, anton.ivanov, johannes,
	linux-um

On Sun, Jun 08, 2025 at 03:00:06AM +0530, Brahmajit Das 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.
> 
> This modification is mainly due to the concerns on
> https://github.com/KSPP/linux/issues/336, where it was mentioned that
> strncpy_from_user is confusingly named as it does not NUL-pad the
> destination, but it does NOT guarantee NUL-termination.
> With this approach/patch we can always ensure that the dst buffer is NUL
> terminated.

I think we need wholesale fix the kernel's usage of
strncpy_from_user()... first, how is it being used? Are things being
manually terminated? Are something destinations not actually C Strings?
We may want two APIs (like strtomem vs strscpy). And then since we're
dealing with user data, I would think padding should be included?

-- 
Kees Cook


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC PATCH] um: replace deprecated strncpy with strscpy
  2025-06-10  4:15 ` Kees Cook
@ 2025-06-12 16:09   ` Brahmajit Das
  0 siblings, 0 replies; 3+ messages in thread
From: Brahmajit Das @ 2025-06-12 16:09 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-hardening, justinstitt, richard, anton.ivanov, johannes,
	linux-um

On 09.06.2025 21:15, Kees Cook wrote:
> 
> I think we need wholesale fix the kernel's usage of
> strncpy_from_user()... first, how is it being used?
So I looked at some of the codes under driver, but it's not very clear
to me how they are currently used.
For example in drivers/tty/vt/vt.c there is this code

	if (!op->data)
		s = NULL;
	else if (strncpy_from_user(name, op->data, MAX_FONT_NAME - 1) < 0)
		return -EFAULT;
	else
		name[MAX_FONT_NAME - 1] = 0;

Which looks like it's not getting NUL terminated.
Where as there's also other instances such as in
drivers/accel/ivpu/ivpu_debugfs.c


	ret = strncpy_from_user(buffer, user_buf, size);
	if (ret < 0)
		return ret;

and in drivers/gpu/drm/virtio/virtgpu_ioctl.c


	ret = strncpy_from_user(vfpriv->debug_name,
			u64_to_user_ptr(value),
			DEBUG_NAME_MAX_LEN - 1);

again in drivers/dma/xilinx/xilinx_dpdma.c we see


	ret = strncpy_from_user(kern_buff, buf, size);
	if (ret < 0)
		goto done;

So it seems like it's not very consistent to me. But I might be wrong.

> Are things being
> manually terminated? Are something destinations not actually C Strings?

Some of them are, while others are not. For example in
drivers/dma/xilinx/xilinx_dpdma.c example the kern_buff is a pointer to
char while in the drivers/gpu/drm/virtio/virtgpu_ioctl.c example
vfpriv->debug_name is an array.

> We may want two APIs (like strtomem vs strscpy). And then since we're
> dealing with user data, I would think padding should be included?

Can you please example on this a bit, like would you like two separate
functions, for example strscpy_from_user and strtomem_from_user. For the
padding we use _pad version of the strscpy function.
> -- 
> Kees Cook

-- 
Regards,
listout


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-06-12 19:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-07 21:30 [RFC PATCH] um: replace deprecated strncpy with strscpy Brahmajit Das
2025-06-10  4:15 ` Kees Cook
2025-06-12 16:09   ` Brahmajit Das

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 NNTP newsgroup(s).