On Tue, 26 Apr 2022, Andy Shevchenko wrote:
> On Sun, Apr 24, 2022 at 10:48 PM Mikulas Patocka <[email protected]> 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 <[email protected]>
> > Cc: [email protected]
>
> 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 <[email protected]>
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 <[email protected]>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: [email protected]
---
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;
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]>
> > > Cc: [email protected]
> >
> > 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
On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <[email protected]> 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 <[email protected]> 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
On Wed, 27 Apr 2022, Andy Shevchenko wrote:
> On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <[email protected]> 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 <[email protected]> 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
On Wed, Apr 27, 2022 at 4:10 PM Mikulas Patocka <[email protected]> wrote:
> On Wed, 27 Apr 2022, Andy Shevchenko wrote:
> > On Tue, Apr 26, 2022 at 5:29 PM Mikulas Patocka <[email protected]> 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 <[email protected]> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Fixes: b78049831ffe ("lib: add error checking to hex2bin")
Cc: [email protected]
---
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;