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

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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.

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:
> > 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@linux.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>