| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VcXNQKej_n4cOB8-gYDSL8MuGrv=Fs8=zdEKGp53DCtSA@mail.gmail.com>
Date: Mon, 18 Jul 2011 21:57:00 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
linux-security-module@...r.kernel.org,
andriy.shevchenko@...ux.intel.com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add error check to hex2bin().
On Mon, Jul 18, 2011 at 9:03 PM, Mimi Zohar <zohar@...ux.vnet.ibm.com> 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@...ove.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@...ux.intel.com>
>> > Intel Finland Oy
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> the body of a message to majordomo@...r.kernel.org
>> 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@...r.kernel.org
> 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists