Re: [PATCH] Add error check to hex2bin().
From: Mimi Zohar <hidden>
Date: 2011-07-18 18:04:05
Also in:
lkml
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().
quoted hunk ↗ jump to hunk
---------------------------------------- [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.
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++);
*dst++ = (c << 4) | d;
if (c < 0 || d < 0)
ret = false;
}
return ret;
thanks,
Mimi
In message "Re: [PATCH] net: can: remove custom hex_to_bin()", Andy Shevchenko wrote:quoted
On Mon, 2011-07-18 at 20:41 +0900, Tetsuo Handa wrote:quoted
Andy Shevchenko wrote:quoted
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 [off-list ref] Intel Finland Oy-- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html