All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hex2bin: fix access beyond string end
@ 2022-04-24 20:48 ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-24 20:48 UTC (permalink / raw)
  To: Linus Torvalds, Andy Shevchenko, Mimi Zohar
  Cc: dm-devel, linux-kernel, Mike Snitzer, Milan Broz

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (hi < 0)
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (lo < 0)
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH] hex2bin: fix access beyond string end
@ 2022-04-24 20:48 ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-24 20:48 UTC (permalink / raw)
  To: Linus Torvalds, Andy Shevchenko, Mimi Zohar
  Cc: dm-devel, Mike Snitzer, linux-kernel, Milan Broz

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (hi < 0)
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (lo < 0)
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] hex2bin: fix access beyond string end
  2022-04-24 20:48 ` [dm-devel] " Mikulas Patocka
@ 2022-04-26 10:37   ` Andy Shevchenko
  -1 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 10:37 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz

On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> If we pass too short string to "hex2bin" (and the string size without the
> terminating NUL character is even), "hex2bin" reads one byte after the
> terminating NUL character. This patch fixes it.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org

You need to provide a Fixes tag.

...

>         while (count--) {
> -               int hi = hex_to_bin(*src++);
> -               int lo = hex_to_bin(*src++);
> +               int hi, lo;
>
> -               if ((hi < 0) || (lo < 0))
> +               hi = hex_to_bin(*src++);
> +               if (hi < 0)
> +                       return -EINVAL;

return hi;

> +               lo = hex_to_bin(*src++);
> +               if (lo < 0)
>                         return -EINVAL;

return lo;

>                 *dst++ = (hi << 4) | lo;

And on top of that it would be nice to understand if we need to
support half-bytes, but in any case it's not a scope of the patch
right now.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH] hex2bin: fix access beyond string end
@ 2022-04-26 10:37   ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 10:37 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz

On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> If we pass too short string to "hex2bin" (and the string size without the
> terminating NUL character is even), "hex2bin" reads one byte after the
> terminating NUL character. This patch fixes it.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org

You need to provide a Fixes tag.

...

>         while (count--) {
> -               int hi = hex_to_bin(*src++);
> -               int lo = hex_to_bin(*src++);
> +               int hi, lo;
>
> -               if ((hi < 0) || (lo < 0))
> +               hi = hex_to_bin(*src++);
> +               if (hi < 0)
> +                       return -EINVAL;

return hi;

> +               lo = hex_to_bin(*src++);
> +               if (lo < 0)
>                         return -EINVAL;

return lo;

>                 *dst++ = (hi << 4) | lo;

And on top of that it would be nice to understand if we need to
support half-bytes, but in any case it's not a scope of the patch
right now.

-- 
With Best Regards,
Andy Shevchenko

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH v2] hex2bin: fix access beyond string end
  2022-04-26 10:37   ` [dm-devel] " Andy Shevchenko
@ 2022-04-26 12:07     ` Mikulas Patocka
  -1 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-26 12:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz



On Tue, 26 Apr 2022, Andy Shevchenko wrote:

> On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > If we pass too short string to "hex2bin" (and the string size without the
> > terminating NUL character is even), "hex2bin" reads one byte after the
> > terminating NUL character. This patch fixes it.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> 
> You need to provide a Fixes tag.

OK. Here I resend it with the "Fixes" tag.

> And on top of that it would be nice to understand if we need to
> support half-bytes, but in any case it's not a scope of the patch
> right now.

Do you think that there are any users who need this?

> -- 
> With Best Regards,
> Andy Shevchenko

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (hi < 0)
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (lo < 0)
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-26 12:07     ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-26 12:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz



On Tue, 26 Apr 2022, Andy Shevchenko wrote:

> On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > If we pass too short string to "hex2bin" (and the string size without the
> > terminating NUL character is even), "hex2bin" reads one byte after the
> > terminating NUL character. This patch fixes it.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> 
> You need to provide a Fixes tag.

OK. Here I resend it with the "Fixes" tag.

> And on top of that it would be nice to understand if we need to
> support half-bytes, but in any case it's not a scope of the patch
> right now.

Do you think that there are any users who need this?

> -- 
> With Best Regards,
> Andy Shevchenko

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (hi < 0)
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (lo < 0)
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-26 12:07     ` [dm-devel] " Mikulas Patocka
@ 2022-04-26 13:18       ` Andy Shevchenko
  -1 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 13:18 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz

On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > >
> > > If we pass too short string to "hex2bin" (and the string size without the
> > > terminating NUL character is even), "hex2bin" reads one byte after the
> > > terminating NUL character. This patch fixes it.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Cc: stable@vger.kernel.org
> > 
> > You need to provide a Fixes tag.
> 
> OK. Here I resend it with the "Fixes" tag.
> 
> > And on top of that it would be nice to understand if we need to
> > support half-bytes, but in any case it's not a scope of the patch
> > right now.
> 
> Do you think that there are any users who need this?

If my memory doesn't do any tricks with me, I have seen the patterns like
hex2bin() + hex_to_bin() in some places in the kernel.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-26 13:18       ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 13:18 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz

On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > >
> > > If we pass too short string to "hex2bin" (and the string size without the
> > > terminating NUL character is even), "hex2bin" reads one byte after the
> > > terminating NUL character. This patch fixes it.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Cc: stable@vger.kernel.org
> > 
> > You need to provide a Fixes tag.
> 
> OK. Here I resend it with the "Fixes" tag.
> 
> > And on top of that it would be nice to understand if we need to
> > support half-bytes, but in any case it's not a scope of the patch
> > right now.
> 
> Do you think that there are any users who need this?

If my memory doesn't do any tricks with me, I have seen the patterns like
hex2bin() + hex_to_bin() in some places in the kernel.

-- 
With Best Regards,
Andy Shevchenko


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-26 12:07     ` [dm-devel] " Mikulas Patocka
@ 2022-04-26 13:19       ` Andy Shevchenko
  -1 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 13:19 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz

On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:

...

> > You need to provide a Fixes tag.
> 
> OK. Here I resend it with the "Fixes" tag.

Still shadows error codes.

> +			return -EINVAL;

>  			return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-26 13:19       ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-26 13:19 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz

On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:

...

> > You need to provide a Fixes tag.
> 
> OK. Here I resend it with the "Fixes" tag.

Still shadows error codes.

> +			return -EINVAL;

>  			return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-26 13:19       ` [dm-devel] " Andy Shevchenko
@ 2022-04-26 15:29         ` Mikulas Patocka
  -1 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-26 15:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz



On Tue, 26 Apr 2022, Andy Shevchenko wrote:

> On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> ...
> 
> > > You need to provide a Fixes tag.
> > 
> > OK. Here I resend it with the "Fixes" tag.
> 
> Still shadows error codes.
> 
> > +			return -EINVAL;
> 
> >  			return -EINVAL;

What do you mean? What's wrong with "return -EINVAL"?

Mikulas


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-26 15:29         ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-26 15:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz



On Tue, 26 Apr 2022, Andy Shevchenko wrote:

> On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> ...
> 
> > > You need to provide a Fixes tag.
> > 
> > OK. Here I resend it with the "Fixes" tag.
> 
> Still shadows error codes.
> 
> > +			return -EINVAL;
> 
> >  			return -EINVAL;

What do you mean? What's wrong with "return -EINVAL"?

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-26 15:29         ` [dm-devel] " Mikulas Patocka
@ 2022-04-27 12:24           ` Andy Shevchenko
  -1 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-27 12:24 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz

On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:

...

> > Still shadows error codes.
> >
> > > +                   return -EINVAL;
> >
> > >                     return -EINVAL;
>
> What do you mean? What's wrong with "return -EINVAL"?

The actual error code is returned by hex_to_bin(). What is the point
of shadowing it with the explicit value?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-27 12:24           ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-27 12:24 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz

On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:

...

> > Still shadows error codes.
> >
> > > +                   return -EINVAL;
> >
> > >                     return -EINVAL;
>
> What do you mean? What's wrong with "return -EINVAL"?

The actual error code is returned by hex_to_bin(). What is the point
of shadowing it with the explicit value?

-- 
With Best Regards,
Andy Shevchenko

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-27 12:24           ` [dm-devel] " Andy Shevchenko
@ 2022-04-27 14:10             ` Mikulas Patocka
  -1 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-27 14:10 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz



On Wed, 27 Apr 2022, Andy Shevchenko wrote:

> On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> ...
> 
> > > Still shadows error codes.
> > >
> > > > +                   return -EINVAL;
> > >
> > > >                     return -EINVAL;
> >
> > What do you mean? What's wrong with "return -EINVAL"?
> 
> The actual error code is returned by hex_to_bin(). What is the point
> of shadowing it with the explicit value?

hex_to_bin returns -1 on error, hex2bin returns -EINVAL on error.

This is inconsistent and it may be fixed (after verifying all the 
hex_to_bin callers - a quick grep over the source shows that there is "if 
((k = hex_to_bin(in_str[j--])) != -1)").

But for the purpose of fixing this bug, we should preserve the behavior 
and return -1 and -EINVAL just like it was before.

Mikulas


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-27 14:10             ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-27 14:10 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz



On Wed, 27 Apr 2022, Andy Shevchenko wrote:

> On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> ...
> 
> > > Still shadows error codes.
> > >
> > > > +                   return -EINVAL;
> > >
> > > >                     return -EINVAL;
> >
> > What do you mean? What's wrong with "return -EINVAL"?
> 
> The actual error code is returned by hex_to_bin(). What is the point
> of shadowing it with the explicit value?

hex_to_bin returns -1 on error, hex2bin returns -EINVAL on error.

This is inconsistent and it may be fixed (after verifying all the 
hex_to_bin callers - a quick grep over the source shows that there is "if 
((k = hex_to_bin(in_str[j--])) != -1)").

But for the purpose of fixing this bug, we should preserve the behavior 
and return -1 and -EINVAL just like it was before.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] hex2bin: fix access beyond string end
  2022-04-27 14:10             ` [dm-devel] " Mikulas Patocka
@ 2022-04-27 14:49               ` Andy Shevchenko
  -1 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-27 14:49 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Linus Torvalds, Andy Shevchenko, Mimi Zohar,
	device-mapper development, Linux Kernel Mailing List,
	Mike Snitzer, Milan Broz

On Wed, Apr 27, 2022 at 4:10 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> On Wed, 27 Apr 2022, Andy Shevchenko wrote:
> > On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > ...
> >
> > > > Still shadows error codes.
> > > >
> > > > > +                   return -EINVAL;
> > > >
> > > > >                     return -EINVAL;
> > >
> > > What do you mean? What's wrong with "return -EINVAL"?
> >
> > The actual error code is returned by hex_to_bin(). What is the point
> > of shadowing it with the explicit value?
>
> hex_to_bin returns -1 on error, hex2bin returns -EINVAL on error.
>
> This is inconsistent and it may be fixed (after verifying all the
> hex_to_bin callers - a quick grep over the source shows that there is "if
> ((k = hex_to_bin(in_str[j--])) != -1)").
>
> But for the purpose of fixing this bug, we should preserve the behavior
> and return -1 and -EINVAL just like it was before.

This is clear now to me. So, by improving a commit message you may
make it clear to everybody who reads your change.

With the updated commit message,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH v2] hex2bin: fix access beyond string end
@ 2022-04-27 14:49               ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-27 14:49 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Andy Shevchenko, Mike Snitzer, Linux Kernel Mailing List,
	Mimi Zohar, device-mapper development, Linus Torvalds,
	Milan Broz

On Wed, Apr 27, 2022 at 4:10 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> On Wed, 27 Apr 2022, Andy Shevchenko wrote:
> > On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > On Tue, Apr 26, 2022 at 08:07:44AM -0400, Mikulas Patocka wrote:
> > > > > On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> > > > > > On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > ...
> >
> > > > Still shadows error codes.
> > > >
> > > > > +                   return -EINVAL;
> > > >
> > > > >                     return -EINVAL;
> > >
> > > What do you mean? What's wrong with "return -EINVAL"?
> >
> > The actual error code is returned by hex_to_bin(). What is the point
> > of shadowing it with the explicit value?
>
> hex_to_bin returns -1 on error, hex2bin returns -EINVAL on error.
>
> This is inconsistent and it may be fixed (after verifying all the
> hex_to_bin callers - a quick grep over the source shows that there is "if
> ((k = hex_to_bin(in_str[j--])) != -1)").
>
> But for the purpose of fixing this bug, we should preserve the behavior
> and return -1 and -EINVAL just like it was before.

This is clear now to me. So, by improving a commit message you may
make it clear to everybody who reads your change.

With the updated commit message,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH v3] hex2bin: fix access beyond string end
  2022-04-27 14:49               ` [dm-devel] " Andy Shevchenko
@ 2022-04-27 15:26                 ` Mikulas Patocka
  -1 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-27 15:26 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andy Shevchenko, Mimi Zohar, device-mapper development,
	Linux Kernel Mailing List, Mike Snitzer, Milan Broz

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Note that hex_to_bin returns -1 on error and hex2bin return -EINVAL on
error - so we can't just return the variable "hi" or "lo" on error. This
inconsistency may be fixed in the next merge window, but for the purpose
of fixing this bug, we just preserve the existing behavior and return -1
and -EINVAL.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-27 17:16:38.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (unlikely(hi < 0))
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (unlikely(lo < 0))
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH v3] hex2bin: fix access beyond string end
@ 2022-04-27 15:26                 ` Mikulas Patocka
  0 siblings, 0 replies; 20+ messages in thread
From: Mikulas Patocka @ 2022-04-27 15:26 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Mike Snitzer, device-mapper development, Mimi Zohar,
	Linux Kernel Mailing List, Andy Shevchenko, Milan Broz

If we pass too short string to "hex2bin" (and the string size without the
terminating NUL character is even), "hex2bin" reads one byte after the
terminating NUL character. This patch fixes it.

Note that hex_to_bin returns -1 on error and hex2bin return -EINVAL on
error - so we can't just return the variable "hi" or "lo" on error. This
inconsistency may be fixed in the next merge window, but for the purpose
of fixing this bug, we just preserve the existing behavior and return -1
and -EINVAL.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: stable@vger.kernel.org

---
 lib/hexdump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6/lib/hexdump.c
===================================================================
--- linux-2.6.orig/lib/hexdump.c	2022-04-24 18:51:16.000000000 +0200
+++ linux-2.6/lib/hexdump.c	2022-04-27 17:16:38.000000000 +0200
@@ -45,10 +45,13 @@ EXPORT_SYMBOL(hex_to_bin);
 int hex2bin(u8 *dst, const char *src, size_t count)
 {
 	while (count--) {
-		int hi = hex_to_bin(*src++);
-		int lo = hex_to_bin(*src++);
+		int hi, lo;
 
-		if ((hi < 0) || (lo < 0))
+		hi = hex_to_bin(*src++);
+		if (unlikely(hi < 0))
+			return -EINVAL;
+		lo = hex_to_bin(*src++);
+		if (unlikely(lo < 0))
 			return -EINVAL;
 
 		*dst++ = (hi << 4) | lo;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-04-29  8:24 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-24 20:48 [PATCH] hex2bin: fix access beyond string end Mikulas Patocka
2022-04-24 20:48 ` [dm-devel] " Mikulas Patocka
2022-04-26 10:37 ` Andy Shevchenko
2022-04-26 10:37   ` [dm-devel] " Andy Shevchenko
2022-04-26 12:07   ` [PATCH v2] " Mikulas Patocka
2022-04-26 12:07     ` [dm-devel] " Mikulas Patocka
2022-04-26 13:18     ` Andy Shevchenko
2022-04-26 13:18       ` [dm-devel] " Andy Shevchenko
2022-04-26 13:19     ` Andy Shevchenko
2022-04-26 13:19       ` [dm-devel] " Andy Shevchenko
2022-04-26 15:29       ` Mikulas Patocka
2022-04-26 15:29         ` [dm-devel] " Mikulas Patocka
2022-04-27 12:24         ` Andy Shevchenko
2022-04-27 12:24           ` [dm-devel] " Andy Shevchenko
2022-04-27 14:10           ` Mikulas Patocka
2022-04-27 14:10             ` [dm-devel] " Mikulas Patocka
2022-04-27 14:49             ` Andy Shevchenko
2022-04-27 14:49               ` [dm-devel] " Andy Shevchenko
2022-04-27 15:26               ` [PATCH v3] " Mikulas Patocka
2022-04-27 15:26                 ` [dm-devel] " Mikulas Patocka

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.