diff mbox series

[v4,1/4] lib/ucs2_string: Add UCS-2 strlcpy function

Message ID 20230528230351.168210-2-luzmaximilian@gmail.com
State New
Headers
Series firmware: Add support for Qualcomm UEFI Secure Application |

Commit Message

Maximilian Luz May 28, 2023, 11:03 p.m. UTC 
  Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
equivalent to the standard strlcpy() function, just for 16-bit character
UCS-2 strings.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Patch introduced in v4

---
 include/linux/ucs2_string.h |  1 +
 lib/ucs2_string.c           | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

Comments

Kees Cook May 30, 2023, 3:25 p.m. UTC | #1 
On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> equivalent to the standard strlcpy() function, just for 16-bit character
> UCS-2 strings.

Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
strscpy() (i.e. use strnlen(), negative error on truncation, etc).
Additionally, it'd be nice of the ucs2 helpers here also implemented the
rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
and destination buffer size overflows at compile-time and run-time with
__builtin_object_size() and __builtin_dynamoc_object_size() respectively).

-Kees

[1] https://docs.kernel.org/process/deprecated.html#strlcpy
Maximilian Luz May 30, 2023, 4:15 p.m. UTC | #2 
On 5/30/23 17:25, Kees Cook wrote:
> On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
>> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
>> equivalent to the standard strlcpy() function, just for 16-bit character
>> UCS-2 strings.
> 
> Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> strscpy() (i.e. use strnlen(), negative error on truncation, etc).

Right, make sense, thanks. Somehow I missed that the kernel has a better
function than the C stdlib for that...

> Additionally, it'd be nice of the ucs2 helpers here also implemented the
> rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> and destination buffer size overflows at compile-time and run-time with
> __builtin_object_size() and __builtin_dynamoc_object_size() respectively).

I can certainly try that, but I think this might be better suited for a
follow-up series, given that we then should also add those to the other
helpers.

Regards,
Max
Ard Biesheuvel May 30, 2023, 4:17 p.m. UTC | #3 
On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 5/30/23 17:25, Kees Cook wrote:
> > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> >> equivalent to the standard strlcpy() function, just for 16-bit character
> >> UCS-2 strings.
> >
> > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> > strscpy() (i.e. use strnlen(), negative error on truncation, etc).
>
> Right, make sense, thanks. Somehow I missed that the kernel has a better
> function than the C stdlib for that...
>
> > Additionally, it'd be nice of the ucs2 helpers here also implemented the
> > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> > and destination buffer size overflows at compile-time and run-time with
> > __builtin_object_size() and __builtin_dynamoc_object_size() respectively).
>
> I can certainly try that, but I think this might be better suited for a
> follow-up series, given that we then should also add those to the other
> helpers.
>

Agreed. Let's log the followup work as a kspp work item, no need to
make that part of this series.

Thanks,
Kees Cook May 30, 2023, 11:18 p.m. UTC | #4 
On Tue, May 30, 2023 at 06:17:35PM +0200, Ard Biesheuvel wrote:
> On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >
> > On 5/30/23 17:25, Kees Cook wrote:
> > > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> > >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> > >> equivalent to the standard strlcpy() function, just for 16-bit character
> > >> UCS-2 strings.
> > >
> > > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> > > strscpy() (i.e. use strnlen(), negative error on truncation, etc).
> >
> > Right, make sense, thanks. Somehow I missed that the kernel has a better
> > function than the C stdlib for that...
> >
> > > Additionally, it'd be nice of the ucs2 helpers here also implemented the
> > > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> > > and destination buffer size overflows at compile-time and run-time with
> > > __builtin_object_size() and __builtin_dynamoc_object_size() respectively).
> >
> > I can certainly try that, but I think this might be better suited for a
> > follow-up series, given that we then should also add those to the other
> > helpers.
> >
> 
> Agreed. Let's log the followup work as a kspp work item, no need to
> make that part of this series.

Yeah, that's fine. Can you please open a KSSP issue for it so we don't
forget?  :)
diff mbox series

Patch

diff --git a/include/linux/ucs2_string.h b/include/linux/ucs2_string.h
index cf3ada3e820e..ffd2a3ed84bb 100644
--- a/include/linux/ucs2_string.h
+++ b/include/linux/ucs2_string.h
@@ -10,6 +10,7 @@  typedef u16 ucs2_char_t;
 unsigned long ucs2_strnlen(const ucs2_char_t *s, size_t maxlength);
 unsigned long ucs2_strlen(const ucs2_char_t *s);
 unsigned long ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength);
+unsigned long ucs2_strlcpy(ucs2_char_t *dst, const ucs2_char_t *src, unsigned long size);
 int ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len);
 
 unsigned long ucs2_utf8size(const ucs2_char_t *src);
diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c
index 0a559a42359b..f474c6b2fe9e 100644
--- a/lib/ucs2_string.c
+++ b/lib/ucs2_string.c
@@ -32,6 +32,22 @@  ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength)
 }
 EXPORT_SYMBOL(ucs2_strsize);
 
+unsigned long
+ucs2_strlcpy(ucs2_char_t *dst, const ucs2_char_t *src, unsigned long size)
+{
+	unsigned long ret = ucs2_strlen(src);
+	unsigned long len;
+
+	if (size) {
+		len = (ret >= size) ? size - 1 : ret;
+		memcpy(dst, src, len * sizeof(*src));
+		dst[len] = 0;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(ucs2_strlcpy);
+
 int
 ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len)
 {