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

On Mon, 2011-07-18 at 21:57 +0300, Andy Shevchenko wrote:

On Mon, Jul 18, 2011 at 9:03 PM, Mimi Zohar [off-list ref] wrote:

quoted

On Mon, 2011-07-18 at 21:48 +0900, Tetsuo Handa wrote:

quoted

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

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

quoted

+             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

quoted

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

quoted

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

quoted

       }
       return ret;