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

[Andy Shevchenko]

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.

>
> 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.

> Â Â Â Â Â Â Â Â*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;
>
> thanks,
>
> Mimi
>
>> In message "Re: [PATCH] net: can: remove custom hex_to_bin()",
>> Andy Shevchenko wrote:
>> > On Mon, 2011-07-18 at 20:41 +0900, Tetsuo Handa wrote:
>> > > Andy Shevchenko wrote:
>> > > > Â Â Â Â for (i = 0, dlc_pos++; i < cf.can_dlc; i++) {
>> > > > -
>> > > > - Â Â Â Â Â Â Â tmp = asc2nibble(sl->rbuff[dlc_pos++]);
>> > > > - Â Â Â Â Â Â Â if (tmp > 0x0F)
>> > > > + Â Â Â Â Â Â Â tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
>> > > > + Â Â Â Â Â Â Â if (tmp < 0)
>> > > > Â Â Â Â Â Â Â Â Â Â Â Â return;
>> > > > Â Â Â Â Â Â Â Â cf.data[i] = (tmp << 4);
>> > > > - Â Â Â Â Â Â Â tmp = asc2nibble(sl->rbuff[dlc_pos++]);
>> > > > - Â Â Â Â Â Â Â if (tmp > 0x0F)
>> > > > + Â Â Â Â Â Â Â tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
>> > > > + Â Â Â Â Â Â Â if (tmp < 0)
>> > > > Â Â Â Â Â Â Â Â Â Â Â Â return;
>> > > > Â Â Â Â Â Â Â Â cf.data[i] |= tmp;
>> > > > Â Â Â Â }
>> > >
>> > > What about changing
>> > >
>> > > Â void hex2bin(u8 *dst, const char *src, size_t count)
>> > >
>> > > to
>> > >
>> > > Â bool hex2bin(u8 *dst, const char *src, size_t count)
>> > >
>> > > in order to do error checks like
>> > >
>> > > bool hex2bin_with_validation(u8 *dst, const char *src, size_t count)
>> > > {
>> > > Â while (count--) {
>> > > Â Â Â Â Â int c = hex_to_bin(*src++);
>> > > Â Â Â Â Â int d;
>> > > Â Â Â Â Â if (c < 0)
>> > > Â Â Â Â Â Â Â Â Â return false;
>> > > Â Â Â Â Â d = hex_to_bin(*src++)
>> > > Â Â Â Â Â if (d < 0)
>> > > Â Â Â Â Â Â Â Â Â return false;
>> > > Â Â Â Â Â *dst++ = (c << 4) | d;
>> > > Â }
>> > > Â return true;
>> > > }
>> > >
>> > > and use hex2bin() rather than hex_to_bin()?
>> > Perhaps, good idea. Could you submit a patch?
>> >
>> > --
>> > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> > Intel Finland Oy
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>>
>
>
> --
> 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/
>



-- 
With Best Regards,
Andy Shevchenko
--
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/