Re: [PATCH] Add error check to hex2bin().

From: Mimi Zohar
Date: Mon Jul 18 2011 - 15:22:31 EST


On Mon, 2011-07-18 at 21:57 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 9:03 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > On Mon, 2011-07-18 at 21:48 +0900, Tetsuo Handa wrote:
> >> Currently, security/keys/ is the only user of hex2bin().
> >> Should I keep hex2bin() unmodified in case of bad input?
> >> If so, I'd like to make it as hex2bin_safe().
> >
> >> ----------------------------------------
> >> [PATCH] Add error check to hex2bin().
> >>
> >> Since converting 2 hexadecimal letters into a byte with error checks is
> >> commonly used, we can replace multiple hex_to_bin() calls with single hex2bin()
> >> call by changing hex2bin() to do error checks.
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I=love.SAKURA.ne.jp>
> >> ---
> >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> >> index 953352a..186e9fc 100644
> >> --- a/include/linux/kernel.h
> >> +++ b/include/linux/kernel.h
> >> @@ -374,7 +374,7 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
> >> }
> >>
> >> extern int hex_to_bin(char ch);
> >> -extern void hex2bin(u8 *dst, const char *src, size_t count);
> >> +extern bool hex2bin(u8 *dst, const char *src, size_t count);
> >>
> >> /*
> >> * General tracing related utility functions - trace_printk(),
> >> diff --git a/lib/hexdump.c b/lib/hexdump.c
> >> index f5fe6ba..1524002 100644
> >> --- a/lib/hexdump.c
> >> +++ b/lib/hexdump.c
> >> @@ -38,14 +38,22 @@ EXPORT_SYMBOL(hex_to_bin);
> >> * @dst: binary result
> >> * @src: ascii hexadecimal string
> >> * @count: result length
> >> + *
> >> + * Returns true on success, false in case of bad input.
> >> */
> >> -void hex2bin(u8 *dst, const char *src, size_t count)
> >> +bool hex2bin(u8 *dst, const char *src, size_t count)
> >> {
> >> while (count--) {
> >> - *dst = hex_to_bin(*src++) << 4;
> >> - *dst += hex_to_bin(*src++);
> >> - dst++;
> >> + int c = hex_to_bin(*src++);
> >> + int d;
> >
> > Missing blank line here.
> >
> >> + if (c < 0)
> >> + return false;
> >> + d = hex_to_bin(*src++);
> >> + if (d < 0)
> >> + return false;
> >> + *dst++ = (c << 4) | d;
> >> }
> >> + return true;
> >> }
> >> EXPORT_SYMBOL(hex2bin);
> >
> > We probably don't need to define a separate 'safe' function.
> There is an opponent on any approach. Although, small and fast error
> route could be good.

As nothing but trusted/encrypted keys is using hex2bin, it shouldn't be
a problem. :-) I'll update trusted/encrypted keys to check the return
code.

thanks,

Mimi

> > Instead of changing the existing code to short circuit out and return a
> > value, does only adding the return value work? Something like:
> >
> > bool ret = true;
> > int c, d;
> >
> > while (count--) {
> > c = hex_to_bin(*src++);
> > d = hex_to_bin(*src++);
> Here is a performance issue, yeah. The user prefers to know about an
> error as soon as possible.

ok

> > *dst++ = (c << 4) | d;
> >
> > if (c < 0 || d < 0)
> > ret = false;
> The ret value is redundant, and here you continue to fill the result
> array by something arbitrary (might be wrong data).

>
> > }
> > return ret;
> >


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/