[PATCH 0/3] net: use strnstr() for lwip_strnstr()

[PATCH 0/3] net: use strnstr() for lwip_strnstr()

[Heinrich Schuchardt]

Using strstr() instead of strnstr() creates a security concern.

* Implement missing library function strnstr() and add unit tests.
* Use it for lwIP.

Heinrich Schuchardt (3):
  lib: implement strnstr()
  test: unit tests for strstr() and strnstr()
  net: use strnstr() for lwip_strnstr()

 include/linux/string.h    |  3 +++
 lib/lwip/u-boot/arch/cc.h |  2 +-
 lib/string.c              | 49 +++++++++++++++++++++++++++------------
 test/lib/string.c         | 40 ++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 16 deletions(-)

-- 
2.47.1

[PATCH 1/3] lib: implement strnstr()

[Heinrich Schuchardt]

Implement library function strnstr().
Implement strstr() using strnstr().
Sort the includes.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/linux/string.h |  3 +++
 lib/string.c           | 49 +++++++++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 27b2beb9ddb..8a377a8176a 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -72,6 +72,9 @@ extern char * strrchr(const char *,int);
 #ifndef __HAVE_ARCH_STRSTR
 extern char * strstr(const char *,const char *);
 #endif
+#ifndef __HAVE_ARCH_STRNSTR
+extern char *strnstr(const char *, const char *, size_t);
+#endif
 #ifndef __HAVE_ARCH_STRLEN
 extern __kernel_size_t strlen(const char *);
 #endif
diff --git a/lib/string.c b/lib/string.c
index feae9519f2f..d5a486fb89d 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -16,11 +16,12 @@
  */
 
 #include <config.h>
+#include <malloc.h>
+#include <stdint.h>
 #include <linux/compiler.h>
+#include <linux/ctype.h>
 #include <linux/types.h>
 #include <linux/string.h>
-#include <linux/ctype.h>
-#include <malloc.h>
 
 /**
  * strncasecmp - Case insensitive, length-limited string comparison
@@ -678,30 +679,48 @@ char *memdup(const void *src, size_t len)
 	return p;
 }
 
-#ifndef __HAVE_ARCH_STRSTR
+#ifndef __HAVE_ARCH_STRNSTR
 /**
- * strstr - Find the first substring in a %NUL terminated string
- * @s1: The string to be searched
- * @s2: The string to search for
+ * strnstr() - find the first substring occurrence in a NUL terminated string
+ *
+ * @s1:		string to be searched
+ * @s2:		string to search for
+ * @len:	maximum number of characters in s2 to consider
+ *
+ * Return:	pointer to the first occurrence or NULL
  */
-char * strstr(const char * s1,const char * s2)
+char *strnstr(const char *s1, const char *s2, size_t len)
 {
-	int l1, l2;
+	size_t l1, l2;
 
+	l1 = strnlen(s1, len);
 	l2 = strlen(s2);
-	if (!l2)
-		return (char *) s1;
-	l1 = strlen(s1);
-	while (l1 >= l2) {
-		l1--;
-		if (!memcmp(s1,s2,l2))
+
+	for (; l1 >= l2; --l1, ++s1) {
+		if (!memcmp(s1, s2, l2))
 			return (char *) s1;
-		s1++;
 	}
+
 	return NULL;
 }
 #endif
 
+#ifndef __HAVE_ARCH_STRSTR
+/**
+ * strstr() - find the first substring occurrence in a NUL terminated string
+ *
+ * @s1:		string to be searched
+ * @s2:		string to search for
+ * @len:	maximum number of characters in s2 to consider
+ *
+ * Return:	pointer to the first occurrence or NULL
+ */
+char *strstr(const char *s1, const char *s2)
+{
+	return strnstr(s1, s2, SIZE_MAX);
+}
+#endif
+
 #ifndef __HAVE_ARCH_MEMCHR
 /**
  * memchr - Find a character in an area of memory.
-- 
2.47.1

[PATCH 2/3] test: unit tests for strstr() and strnstr()

[Heinrich Schuchardt]

Add unit tests for the library functions.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/string.c      |  2 +-
 test/lib/string.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/lib/string.c b/lib/string.c
index d5a486fb89d..66aae583940 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -16,8 +16,8 @@
  */
 
 #include <config.h>
+#include <limits.h>
 #include <malloc.h>
-#include <stdint.h>
 #include <linux/compiler.h>
 #include <linux/ctype.h>
 #include <linux/types.h>
diff --git a/test/lib/string.c b/test/lib/string.c
index 8d22f3fd68f..31391a387b9 100644
--- a/test/lib/string.c
+++ b/test/lib/string.c
@@ -11,6 +11,7 @@
 
 #include <command.h>
 #include <log.h>
+#include <string.h>
 #include <test/lib.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -221,3 +222,42 @@ static int lib_memdup(struct unit_test_state *uts)
 	return 0;
 }
 LIB_TEST(lib_memdup, 0);
+
+/** lib_strnstr() - unit test for strnstr() */
+static int lib_strnstr(struct unit_test_state *uts)
+{
+	const char *s1 = "Itsy Bitsy Teenie Weenie";
+	const char *s2 = "eenie";
+	const char *s3 = "eery";
+
+	ut_asserteq_ptr(&s1[12], strnstr(s1, s2, SIZE_MAX));
+	ut_asserteq_ptr(&s1[12], strnstr(s1, s2, 17));
+	ut_assertnull(strnstr(s1, s2, 16));
+	ut_assertnull(strnstr(s1, s2, 0));
+	ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], SIZE_MAX));
+	ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], 14));
+	ut_assertnull(strnstr(&s1[3], &s2[1], 13));
+	ut_assertnull(strnstr(&s1[3], &s2[1], 0));
+	ut_assertnull(strnstr(s1, s3, SIZE_MAX));
+	ut_assertnull(strnstr(s1, s3, 0));
+
+	return 0;
+}
+LIB_TEST(lib_strnstr, 0);
+
+/** lib_strstr() - unit test for strstr() */
+static int lib_strstr(struct unit_test_state *uts)
+{
+	const char *s1 = "Itsy Bitsy Teenie Weenie";
+	const char *s2 = "eenie";
+	const char *s3 = "easy";
+
+	ut_asserteq_ptr(&s1[12], strstr(s1, s2));
+	ut_asserteq_ptr(&s1[13], strstr(&s1[3], &s2[1]));
+	ut_assertnull(strstr(s1, s3));
+	ut_asserteq_ptr(&s1[2], strstr(s1, &s3[2]));
+	ut_asserteq_ptr(&s1[8], strstr(&s1[5], &s3[2]));
+
+	return 0;
+}
+LIB_TEST(lib_strstr, 0);
-- 
2.47.1

[PATCH 3/3] net: use strnstr() for lwip_strnstr()

[Heinrich Schuchardt]

Using strstr() instead of strnstr() creates a security concern.

Fixes: 1c41a7afaa15 ("net: lwip: build lwIP")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/lwip/u-boot/arch/cc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h
index de138846358..6104c296f6f 100644
--- a/lib/lwip/u-boot/arch/cc.h
+++ b/lib/lwip/u-boot/arch/cc.h
@@ -34,7 +34,7 @@
 	       x, __LINE__, __FILE__); } while (0)
 
 #define atoi(str) (int)dectoul(str, NULL)
-#define lwip_strnstr(a, b, c)  strstr(a, b)
+#define lwip_strnstr(a, b, c)  strnstr(a, b, c)
 
 #define LWIP_ERR_T int
 #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
-- 
2.47.1

Re: [PATCH 1/3] lib: implement strnstr()

[Jerome Forissier]

On 1/4/25 00:21, Heinrich Schuchardt wrote:
> Implement library function strnstr().
> Implement strstr() using strnstr().
> Sort the includes.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/linux/string.h |  3 +++
>  lib/string.c           | 49 +++++++++++++++++++++++++++++-------------
>  2 files changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 27b2beb9ddb..8a377a8176a 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -72,6 +72,9 @@ extern char * strrchr(const char *,int);
>  #ifndef __HAVE_ARCH_STRSTR
>  extern char * strstr(const char *,const char *);
>  #endif
> +#ifndef __HAVE_ARCH_STRNSTR
> +extern char *strnstr(const char *, const char *, size_t);
> +#endif
>  #ifndef __HAVE_ARCH_STRLEN
>  extern __kernel_size_t strlen(const char *);
>  #endif
> diff --git a/lib/string.c b/lib/string.c
> index feae9519f2f..d5a486fb89d 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -16,11 +16,12 @@
>   */
>  
>  #include <config.h>
> +#include <malloc.h>
> +#include <stdint.h>
>  #include <linux/compiler.h>
> +#include <linux/ctype.h>
>  #include <linux/types.h>
>  #include <linux/string.h>
> -#include <linux/ctype.h>
> -#include <malloc.h>
>  
>  /**
>   * strncasecmp - Case insensitive, length-limited string comparison
> @@ -678,30 +679,48 @@ char *memdup(const void *src, size_t len)
>  	return p;
>  }
>  
> -#ifndef __HAVE_ARCH_STRSTR
> +#ifndef __HAVE_ARCH_STRNSTR
>  /**
> - * strstr - Find the first substring in a %NUL terminated string
> - * @s1: The string to be searched
> - * @s2: The string to search for
> + * strnstr() - find the first substring occurrence in a NUL terminated string
> + *
> + * @s1:		string to be searched
> + * @s2:		string to search for
> + * @len:	maximum number of characters in s2 to consider

You mean s1?

> + *
> + * Return:	pointer to the first occurrence or NULL
>   */
> -char * strstr(const char * s1,const char * s2)
> +char *strnstr(const char *s1, const char *s2, size_t len)
>  {
> -	int l1, l2;
> +	size_t l1, l2;
>  
> +	l1 = strnlen(s1, len);
>  	l2 = strlen(s2);
> -	if (!l2)
> -		return (char *) s1;
> -	l1 = strlen(s1);
> -	while (l1 >= l2) {
> -		l1--;
> -		if (!memcmp(s1,s2,l2))
> +
> +	for (; l1 >= l2; --l1, ++s1) {
> +		if (!memcmp(s1, s2, l2))
>  			return (char *) s1;
> -		s1++;
>  	}
> +
>  	return NULL;
>  }
>  #endif

This won't return s1 when s2 == NULL, will it?
Why not use a known good implementation such as [1]?

[1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c

>  
> +#ifndef __HAVE_ARCH_STRSTR
> +/**
> + * strstr() - find the first substring occurrence in a NUL terminated string
> + *
> + * @s1:		string to be searched
> + * @s2:		string to search for
> + * @len:	maximum number of characters in s2 to consider

There is no len parameter

> + *
> + * Return:	pointer to the first occurrence or NULL
> + */
> +char *strstr(const char *s1, const char *s2)
> +{
> +	return strnstr(s1, s2, SIZE_MAX);
> +}
> +#endif
> +
>  #ifndef __HAVE_ARCH_MEMCHR
>  /**
>   * memchr - Find a character in an area of memory.

Thanks,
-- 
Jerome

Re: [PATCH 3/3] net: use strnstr() for lwip_strnstr()

[Jerome Forissier]

On 1/4/25 00:21, Heinrich Schuchardt wrote:
> Using strstr() instead of strnstr() creates a security concern.
> 
> Fixes: 1c41a7afaa15 ("net: lwip: build lwIP")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/lwip/u-boot/arch/cc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h
> index de138846358..6104c296f6f 100644
> --- a/lib/lwip/u-boot/arch/cc.h
> +++ b/lib/lwip/u-boot/arch/cc.h
> @@ -34,7 +34,7 @@
>  	       x, __LINE__, __FILE__); } while (0)
>  
>  #define atoi(str) (int)dectoul(str, NULL)
> -#define lwip_strnstr(a, b, c)  strstr(a, b)
> +#define lwip_strnstr(a, b, c)  strnstr(a, b, c)
>  
>  #define LWIP_ERR_T int
>  #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Re: [PATCH 1/3] lib: implement strnstr()

[Rasmus Villemoes]

On Mon, Jan 06 2025, Jerome Forissier <jerome.forissier@linaro.org> wrote:

>> + *
>> + * Return:	pointer to the first occurrence or NULL
>>   */
>> -char * strstr(const char * s1,const char * s2)
>> +char *strnstr(const char *s1, const char *s2, size_t len)
>>  {
>> -	int l1, l2;
>> +	size_t l1, l2;
>>  
>> +	l1 = strnlen(s1, len);
>>  	l2 = strlen(s2);
>> -	if (!l2)
>> -		return (char *) s1;
>> -	l1 = strlen(s1);
>> -	while (l1 >= l2) {
>> -		l1--;
>> -		if (!memcmp(s1,s2,l2))
>> +
>> +	for (; l1 >= l2; --l1, ++s1) {
>> +		if (!memcmp(s1, s2, l2))
>>  			return (char *) s1;
>> -		s1++;
>>  	}
>> +
>>  	return NULL;
>>  }
>>  #endif
>
> This won't return s1 when s2 == NULL, will it?

Why should it? That's a broken caller. You can't call str* functions
with NULL pointers. It's ok for s2 to point at an empty string, in which
case l2 is 0, so the very first memcmp() is guaranteed to succeed and
yes, this will return s1 in that case.

As passing "" as s2 is likely quite rare, the current short-circuiting
of that case is pointless.

> Why not use a known good implementation such as [1]?
>
> [1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c

Licensing? Also, that's quite unreadable, so please don't.

Rasmus

Re: [PATCH 1/3] lib: implement strnstr()

[Jerome Forissier]

On 1/6/25 11:14, Rasmus Villemoes wrote:
> On Mon, Jan 06 2025, Jerome Forissier <jerome.forissier@linaro.org> wrote:
> 
>>> + *
>>> + * Return:	pointer to the first occurrence or NULL
>>>   */
>>> -char * strstr(const char * s1,const char * s2)
>>> +char *strnstr(const char *s1, const char *s2, size_t len)
>>>  {
>>> -	int l1, l2;
>>> +	size_t l1, l2;
>>>  
>>> +	l1 = strnlen(s1, len);
>>>  	l2 = strlen(s2);
>>> -	if (!l2)
>>> -		return (char *) s1;
>>> -	l1 = strlen(s1);
>>> -	while (l1 >= l2) {
>>> -		l1--;
>>> -		if (!memcmp(s1,s2,l2))
>>> +
>>> +	for (; l1 >= l2; --l1, ++s1) {
>>> +		if (!memcmp(s1, s2, l2))
>>>  			return (char *) s1;
>>> -		s1++;
>>>  	}
>>> +
>>>  	return NULL;
>>>  }
>>>  #endif
>>
>> This won't return s1 when s2 == NULL, will it?
> 
> Why should it? That's a broken caller. You can't call str* functions
> with NULL pointers. It's ok for s2 to point at an empty string, in which
> case l2 is 0, so the very first memcmp() is guaranteed to succeed and
> yes, this will return s1 in that case.

Sorry I read the strncmp() man page too quickly and mistook empty string
for NULL :-/

> 
> As passing "" as s2 is likely quite rare, the current short-circuiting
> of that case is pointless.

Agreed.
 
>> Why not use a known good implementation such as [1]?
>>
>> [1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c
> 
> Licensing?

Isn't BSD compatible with GPL?


> Also, that's quite unreadable, so please don't.

OK.

> Rasmus

-- 
Jerome

Re: [PATCH 2/3] test: unit tests for strstr() and strnstr()

[Ilias Apalodimas]

On Sat, 4 Jan 2025 at 01:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Add unit tests for the library functions.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/string.c      |  2 +-
>  test/lib/string.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/lib/string.c b/lib/string.c
> index d5a486fb89d..66aae583940 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -16,8 +16,8 @@
>   */
>
>  #include <config.h>
> +#include <limits.h>
>  #include <malloc.h>
> -#include <stdint.h>
>  #include <linux/compiler.h>
>  #include <linux/ctype.h>
>  #include <linux/types.h>
> diff --git a/test/lib/string.c b/test/lib/string.c
> index 8d22f3fd68f..31391a387b9 100644
> --- a/test/lib/string.c
> +++ b/test/lib/string.c
> @@ -11,6 +11,7 @@
>
>  #include <command.h>
>  #include <log.h>
> +#include <string.h>
>  #include <test/lib.h>
>  #include <test/test.h>
>  #include <test/ut.h>
> @@ -221,3 +222,42 @@ static int lib_memdup(struct unit_test_state *uts)
>         return 0;
>  }
>  LIB_TEST(lib_memdup, 0);
> +
> +/** lib_strnstr() - unit test for strnstr() */
> +static int lib_strnstr(struct unit_test_state *uts)
> +{
> +       const char *s1 = "Itsy Bitsy Teenie Weenie";
> +       const char *s2 = "eenie";
> +       const char *s3 = "eery";
> +
> +       ut_asserteq_ptr(&s1[12], strnstr(s1, s2, SIZE_MAX));
> +       ut_asserteq_ptr(&s1[12], strnstr(s1, s2, 17));
> +       ut_assertnull(strnstr(s1, s2, 16));
> +       ut_assertnull(strnstr(s1, s2, 0));
> +       ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], SIZE_MAX));
> +       ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], 14));
> +       ut_assertnull(strnstr(&s1[3], &s2[1], 13));
> +       ut_assertnull(strnstr(&s1[3], &s2[1], 0));
> +       ut_assertnull(strnstr(s1, s3, SIZE_MAX));
> +       ut_assertnull(strnstr(s1, s3, 0));
> +
> +       return 0;
> +}
> +LIB_TEST(lib_strnstr, 0);
> +
> +/** lib_strstr() - unit test for strstr() */
> +static int lib_strstr(struct unit_test_state *uts)
> +{
> +       const char *s1 = "Itsy Bitsy Teenie Weenie";
> +       const char *s2 = "eenie";
> +       const char *s3 = "easy";
> +
> +       ut_asserteq_ptr(&s1[12], strstr(s1, s2));
> +       ut_asserteq_ptr(&s1[13], strstr(&s1[3], &s2[1]));
> +       ut_assertnull(strstr(s1, s3));
> +       ut_asserteq_ptr(&s1[2], strstr(s1, &s3[2]));
> +       ut_asserteq_ptr(&s1[8], strstr(&s1[5], &s3[2]));
> +
> +       return 0;
> +}
> +LIB_TEST(lib_strstr, 0);
> --
> 2.47.1
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 0/3] net: use strnstr() for lwip_strnstr()

[Tom Rini]

On Sat, 04 Jan 2025 00:21:15 +0100, Heinrich Schuchardt wrote:

> Using strstr() instead of strnstr() creates a security concern.
> 
> * Implement missing library function strnstr() and add unit tests.
> * Use it for lwIP.
> 
> Heinrich Schuchardt (3):
>   lib: implement strnstr()
>   test: unit tests for strstr() and strnstr()
>   net: use strnstr() for lwip_strnstr()
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom

Re: [PATCH 0/3] net: use strnstr() for lwip_strnstr()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Mon, Jan 20, 2025 at 02:52:24PM -0600, Tom Rini wrote:
> On Sat, 04 Jan 2025 00:21:15 +0100, Heinrich Schuchardt wrote:
> 
> > Using strstr() instead of strnstr() creates a security concern.
> > 
> > * Implement missing library function strnstr() and add unit tests.
> > * Use it for lwIP.
> > 
> > Heinrich Schuchardt (3):
> >   lib: implement strnstr()
> >   test: unit tests for strstr() and strnstr()
> >   net: use strnstr() for lwip_strnstr()
> > 
> > [...]
> 
> Applied to u-boot/master, thanks!

This was premature, sorry, I need to re-test this in Azure before
pushing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] net: use strnstr() for lwip_strnstr()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:

> Using strstr() instead of strnstr() creates a security concern.
> 
> Fixes: 1c41a7afaa15 ("net: lwip: build lwIP")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  lib/lwip/u-boot/arch/cc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h
> index de138846358..6104c296f6f 100644
> --- a/lib/lwip/u-boot/arch/cc.h
> +++ b/lib/lwip/u-boot/arch/cc.h
> @@ -34,7 +34,7 @@
>  	       x, __LINE__, __FILE__); } while (0)
>  
>  #define atoi(str) (int)dectoul(str, NULL)
> -#define lwip_strnstr(a, b, c)  strstr(a, b)
> +#define lwip_strnstr(a, b, c)  strnstr(a, b, c)
>  
>  #define LWIP_ERR_T int
>  #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))

This leads to:
https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=logs&j=45c0c132-56cd-504a-56c7-1b8a534aa92b&t=f95d3367-9db2-5f2a-d3ed-8901d714dd66
as a failure, that I only end up seeing in Azure (I didn't track down if
there's some good reason we don't see this in Gitlab).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] net: use strnstr() for lwip_strnstr()

[Heinrich Schuchardt]

On 21.01.25 02:05, Tom Rini wrote:
> On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:
> 
>> Using strstr() instead of strnstr() creates a security concern.
>>
>> Fixes: 1c41a7afaa15 ("net: lwip: build lwIP")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>   lib/lwip/u-boot/arch/cc.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h
>> index de138846358..6104c296f6f 100644
>> --- a/lib/lwip/u-boot/arch/cc.h
>> +++ b/lib/lwip/u-boot/arch/cc.h
>> @@ -34,7 +34,7 @@
>>   	       x, __LINE__, __FILE__); } while (0)
>>   
>>   #define atoi(str) (int)dectoul(str, NULL)
>> -#define lwip_strnstr(a, b, c)  strstr(a, b)
>> +#define lwip_strnstr(a, b, c)  strnstr(a, b, c)
>>   
>>   #define LWIP_ERR_T int
>>   #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
> 
> This leads to:
> https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=logs&j=45c0c132-56cd-504a-56c7-1b8a534aa92b&t=f95d3367-9db2-5f2a-d3ed-8901d714dd66
> as a failure, that I only end up seeing in Azure (I didn't track down if
> there's some good reason we don't see this in Gitlab).
> 

Hello Tom,

It is not really clear how

     Lab failure: Timeout executing 'tftpboot 40400000 u-boot.bin

could be related to this patch series.

     git grep -ni strstr net/

only shows usage in wget but not in tftp.

Is this test result reproducible in Azure?

Best regards

Heinrich

Re: [PATCH 3/3] net: use strnstr() for lwip_strnstr()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

On Tue, Jan 21, 2025 at 03:18:11AM +0100, Heinrich Schuchardt wrote:
> On 21.01.25 02:05, Tom Rini wrote:
> > On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:
> > 
> > > Using strstr() instead of strnstr() creates a security concern.
> > > 
> > > Fixes: 1c41a7afaa15 ("net: lwip: build lwIP")
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > ---
> > >   lib/lwip/u-boot/arch/cc.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h
> > > index de138846358..6104c296f6f 100644
> > > --- a/lib/lwip/u-boot/arch/cc.h
> > > +++ b/lib/lwip/u-boot/arch/cc.h
> > > @@ -34,7 +34,7 @@
> > >   	       x, __LINE__, __FILE__); } while (0)
> > >   #define atoi(str) (int)dectoul(str, NULL)
> > > -#define lwip_strnstr(a, b, c)  strstr(a, b)
> > > +#define lwip_strnstr(a, b, c)  strnstr(a, b, c)
> > >   #define LWIP_ERR_T int
> > >   #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
> > 
> > This leads to:
> > https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=logs&j=45c0c132-56cd-504a-56c7-1b8a534aa92b&t=f95d3367-9db2-5f2a-d3ed-8901d714dd66
> > as a failure, that I only end up seeing in Azure (I didn't track down if
> > there's some good reason we don't see this in Gitlab).
> > 
> 
> Hello Tom,
> 
> It is not really clear how
> 
>     Lab failure: Timeout executing 'tftpboot 40400000 u-boot.bin
> 
> could be related to this patch series.
> 
>     git grep -ni strstr net/
> 
> only shows usage in wget but not in tftp.

Agreed.

> Is this test result reproducible in Azure?

Yes, every time. And 1/3 + 2/3 pass, with 3/3 is when it fails.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]