[PATCH v2 0/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[PATCH v2 0/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[Vincent Mailhol]

This series fixes an off-by-one out-of-bounds read in ucs2_strnlen().

The first patch is the real fix, the second patch comes as a bonus and
fixes the code indentation.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changes in v2:

  - Toggle the patch order. ucs2_strnlen() is now before the
    tabulation fix to make it easier to backport.

  - Add Cc: stable on the ucs2_strnlen().

Link to v1: https://lore.kernel.org/r/20260722-fix-ucs2_strnlen-v1-0-ba5bade2b026@kernel.org

---
Vincent Mailhol (2):
      lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()
      lib/ucs2_string.c: fix indentation

 lib/ucs2_string.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
---
base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
change-id: 20260722-fix-ucs2_strnlen-145f5d4b1f6b

Best regards,
-- 
Vincent Mailhol <mailhol@kernel.org>

[PATCH v2 1/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[Vincent Mailhol]

ucs2_strnlen() checks the current character before checking whether the
caller-provided maximum length has been reached. If the input is not
NUL-terminated within that bound, the loop can read one ucs2_char_t past
the limit.

Test the length before dereferencing to prevent an off-by-one
out-of-bounds read.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Cc: stable@vger.kernel.org
Cc: Kees Cook <kees@kernel.org>

Concerning the Fixes: tag, the ucs2_strnlen() function can be traced as
follow:

  - commit 0635eb8a54cf ("Move utf16 functions to kernel core and
    rename") renames it from utf16_strnlen() to ucs2_strnlen() as we
    know it today.

  - commit a2940908391f ("efivars: String functions") renames
    utf8_strlen() to utf16_strnlen().

  - utf8_strlen() is present since the begining of the git history in
    commit 1da177e4c3f4 ("Linux-2.6.12-rc2")

And that of-by-one issue was present since the beginning and survived
all the renamings. This is why I picked 1da177e4c3f4 in the Fixes tag.
---
 lib/ucs2_string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c
index dfb4f2358cab..ca5c615d5bef 100644
--- a/lib/ucs2_string.c
+++ b/lib/ucs2_string.c
@@ -8,7 +8,7 @@ ucs2_strnlen(const ucs2_char_t *s, size_t maxlength)
 {
         unsigned long length = 0;
 
-        while (*s++ != 0 && length < maxlength)
+	while (length < maxlength && *s++ != 0)
                 length++;
         return length;
 }

-- 
2.54.0

[PATCH v2 2/2] lib/ucs2_string.c: fix indentation

[Vincent Mailhol]

checkpatch.pl reports a total of 18 incorrect use of spaces instead of
tabulations in lib/ucs2_string.c.

Apply

  ./scripts/checkpatch.pl --fix-inplace lib/ucs2_string.c

to fix them all.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 lib/ucs2_string.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c
index ca5c615d5bef..1f7dd4eb640a 100644
--- a/lib/ucs2_string.c
+++ b/lib/ucs2_string.c
@@ -6,18 +6,18 @@
 unsigned long
 ucs2_strnlen(const ucs2_char_t *s, size_t maxlength)
 {
-        unsigned long length = 0;
+	unsigned long length = 0;
 
 	while (length < maxlength && *s++ != 0)
-                length++;
-        return length;
+		length++;
+	return length;
 }
 EXPORT_SYMBOL(ucs2_strnlen);
 
 unsigned long
 ucs2_strlen(const ucs2_char_t *s)
 {
-        return ucs2_strnlen(s, ~0UL);
+	return ucs2_strnlen(s, ~0UL);
 }
 EXPORT_SYMBOL(ucs2_strlen);
 
@@ -28,7 +28,7 @@ EXPORT_SYMBOL(ucs2_strlen);
 unsigned long
 ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength)
 {
-        return ucs2_strnlen(data, maxlength/sizeof(ucs2_char_t)) * sizeof(ucs2_char_t);
+	return ucs2_strnlen(data, maxlength/sizeof(ucs2_char_t)) * sizeof(ucs2_char_t);
 }
 EXPORT_SYMBOL(ucs2_strsize);
 
@@ -87,19 +87,19 @@ EXPORT_SYMBOL(ucs2_strscpy);
 int
 ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len)
 {
-        while (1) {
-                if (len == 0)
-                        return 0;
-                if (*a < *b)
-                        return -1;
-                if (*a > *b)
-                        return 1;
-                if (*a == 0) /* implies *b == 0 */
-                        return 0;
-                a++;
-                b++;
-                len--;
-        }
+	while (1) {
+		if (len == 0)
+			return 0;
+		if (*a < *b)
+			return -1;
+		if (*a > *b)
+			return 1;
+		if (*a == 0) /* implies *b == 0 */
+			return 0;
+		a++;
+		b++;
+		len--;
+	}
 }
 EXPORT_SYMBOL(ucs2_strncmp);
 

-- 
2.54.0

Re: [PATCH v2 1/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[Greg KH]

On Thu, Jul 23, 2026 at 09:40:31PM +0200, Vincent Mailhol wrote:
> ucs2_strnlen() checks the current character before checking whether the
> caller-provided maximum length has been reached. If the input is not
> NUL-terminated within that bound, the loop can read one ucs2_char_t past
> the limit.
> 
> Test the length before dereferencing to prevent an off-by-one
> out-of-bounds read.

That's fine, but doing that read doesn't actually "hurt" anything,
right?  So this shouldn't be needed in stable kernels.  Or am I missing
something?

thanks,

greg k-h

Re: [PATCH v2 1/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[Vincent Mailhol]

On 24/07/2026 at 07:54, Greg KH wrote:
> On Thu, Jul 23, 2026 at 09:40:31PM +0200, Vincent Mailhol wrote:
>> ucs2_strnlen() checks the current character before checking whether the
>> caller-provided maximum length has been reached. If the input is not
>> NUL-terminated within that bound, the loop can read one ucs2_char_t past
>> the limit.
>>
>> Test the length before dereferencing to prevent an off-by-one
>> out-of-bounds read.
> 
> That's fine, but doing that read doesn't actually "hurt" anything,
> right?  So this shouldn't be needed in stable kernels.  Or am I missing
> something?

I had a similar feeling and didn't CC stable on the v1. But following
my exchange with Andrew here:

  https://lore.kernel.org/all/CAMZ6Rq+Ax5MTs0Ke=XAB2JFX1KXuFas3B2eEMv+nQ4x7up2BLA@mail.gmail.com/

I think there *might* be some risk and finally CCed stable in v2. If
you prefer to not backport, that's OK for me. As you can see in my
above message, even if the risk exists, it is more theoretical and I
don't think it would provide much leverage to turn it into a real
exploit.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 1/2] lib/ucs2_string.c: fix out-of-bounds read in ucs2_strnlen()

[Greg KH]

On Fri, Jul 24, 2026 at 08:59:02AM +0200, Vincent Mailhol wrote:
> On 24/07/2026 at 07:54, Greg KH wrote:
> > On Thu, Jul 23, 2026 at 09:40:31PM +0200, Vincent Mailhol wrote:
> >> ucs2_strnlen() checks the current character before checking whether the
> >> caller-provided maximum length has been reached. If the input is not
> >> NUL-terminated within that bound, the loop can read one ucs2_char_t past
> >> the limit.
> >>
> >> Test the length before dereferencing to prevent an off-by-one
> >> out-of-bounds read.
> > 
> > That's fine, but doing that read doesn't actually "hurt" anything,
> > right?  So this shouldn't be needed in stable kernels.  Or am I missing
> > something?
> 
> I had a similar feeling and didn't CC stable on the v1. But following
> my exchange with Andrew here:
> 
>   https://lore.kernel.org/all/CAMZ6Rq+Ax5MTs0Ke=XAB2JFX1KXuFas3B2eEMv+nQ4x7up2BLA@mail.gmail.com/
> 
> I think there *might* be some risk and finally CCed stable in v2. If
> you prefer to not backport, that's OK for me. As you can see in my
> above message, even if the risk exists, it is more theoretical and I
> don't think it would provide much leverage to turn it into a real
> exploit.

"turning into a real exploit" is not a requirement for stable backports
at all.  I was just thinking "is this even a real bug that needs to be
changed at all" given the lifetime of this issue, and the location from
where these strings come from (i.e. firmware images).

But yes, from a logical "this code should not be doing this" it does
seem correct to make.  Maybe someday we'll even be able to drop these
extra string functions entirely as we already have ways to convert from
utf16 to utf8 in the kernel already, so maybe we should just "normalize"
them all to start with after read from the firmware?

thanks,

greg k-h