[PATCH] net: can: remove custom hex_to_bin()

[PATCH] net: can: remove custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/slcan.c |   26 +++++---------------------
 1 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index aa8ad73..65e54fd 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -56,6 +56,7 @@
 #include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/can.h>
 
 static __initdata const char banner[] =
@@ -142,21 +143,6 @@ static struct net_device **slcan_devs;
   *			STANDARD SLCAN DECAPSULATION			 *
   ************************************************************************/
 
-static int asc2nibble(char c)
-{
-
-	if ((c >= '0') && (c <= '9'))
-		return c - '0';
-
-	if ((c >= 'A') && (c <= 'F'))
-		return c - 'A' + 10;
-
-	if ((c >= 'a') && (c <= 'f'))
-		return c - 'a' + 10;
-
-	return 16; /* error */
-}
-
 /* Send one completely decapsulated can_frame to the network layer */
 static void slc_bump(struct slcan *sl)
 {
@@ -195,18 +181,16 @@ static void slc_bump(struct slcan *sl)
 	*(u64 *) (&cf.data) = 0; /* clear payload */
 
 	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;
 	}
 
-
 	skb = dev_alloc_skb(sizeof(struct can_frame));
 	if (!skb)
 		return;
-- 
1.7.5.4

Re: [PATCH] net: can: remove custom hex_to_bin()

[Tetsuo Handa]

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()?

Re: [PATCH] net: can: remove custom hex_to_bin()

[Andy Shevchenko]

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

[PATCH] Add error check to hex2bin().

[Tetsuo Handa]

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;
+		if (c < 0)
+			return false;
+		d = hex_to_bin(*src++);
+		if (d < 0)
+			return false;
+		*dst++ = (c << 4) | d;
 	}
+	return true;
 }
 EXPORT_SYMBOL(hex2bin);
 


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

Re: [PATCH] net: can: remove custom hex_to_bin()

[Oliver Hartkopp]

On 18.07.2011 10:26, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Wolfgang Grandegger <wg@grandegger.com>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
>  drivers/net/can/slcan.c |   26 +++++---------------------
>  1 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index aa8ad73..65e54fd 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -56,6 +56,7 @@
>  #include <linux/sched.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/can.h>
>  
>  static __initdata const char banner[] =
> @@ -142,21 +143,6 @@ static struct net_device **slcan_devs;
>    *			STANDARD SLCAN DECAPSULATION			 *
>    ************************************************************************/
>  
> -static int asc2nibble(char c)
> -{
> -
> -	if ((c >= '0') && (c <= '9'))
> -		return c - '0';
> -
> -	if ((c >= 'A') && (c <= 'F'))
> -		return c - 'A' + 10;
> -
> -	if ((c >= 'a') && (c <= 'f'))
> -		return c - 'a' + 10;
> -
> -	return 16; /* error */
> -}
> -
>  /* Send one completely decapsulated can_frame to the network layer */
>  static void slc_bump(struct slcan *sl)
>  {
> @@ -195,18 +181,16 @@ static void slc_bump(struct slcan *sl)
>  	*(u64 *) (&cf.data) = 0; /* clear payload */
>  
>  	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;
>  	}
>  
> -
>  	skb = dev_alloc_skb(sizeof(struct can_frame));
>  	if (!skb)
>  		return;

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

[Mimi Zohar]

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
> 

Re: [PATCH] net: can: remove custom hex_to_bin()

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Mon, 18 Jul 2011 16:02:49 +0200

> On 18.07.2011 10:26, Andy Shevchenko wrote:
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Wolfgang Grandegger <wg@grandegger.com>
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Applied.

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

[Andy Shevchenko]

On Mon, Jul 18, 2011 at 9:03 PM, Mimi Zohar <zohar@linux.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@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@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
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.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

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

[Mimi Zohar]

On Mon, 2011-07-18 at 21:57 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 9:03 PM, Mimi Zohar <zohar@linux.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@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.

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

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

> >                *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;
> >

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

[Andy Shevchenko]

On Mon, Jul 18, 2011 at 10:20 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

>> > 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.  :-)
The key word "until now". But people will start to use anything which
has public API, won't they?

>  I'll update trusted/encrypted keys to check the return
> code.
Actually another question shall we add __must_check to the prototype or not?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Jul 18, 2011 at 10:20 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

>> > 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.  :-)
The key word "until now". But people will start to use anything which
has public API, won't they?

>  I'll update trusted/encrypted keys to check the return
> code.
Actually another question shall we add __must_check to the prototype or not?

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

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

[Geert Uytterhoeven]

On Mon, Jul 18, 2011 at 14:48, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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.

What about making it return the number of unprocessed bytes left instead?
Then the caller knows where the problem lies. And zero would mean success.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Mon, Jul 18, 2011 at 14:48, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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.

What about making it return the number of unprocessed bytes left instead?
Then the caller knows where the problem lies. And zero would mean success.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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

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

[Andy Shevchenko]

On Mon, Jul 18, 2011 at 11:49 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> What about making it return the number of unprocessed bytes left instead?
> Then the caller knows where the problem lies. And zero would mean success.
If I remember correctly it used to be src as return value in some
version of that patch. I don't know the details of that interim
solution. My current opinion is to return boolean and make an
additional parameter to return src value. However, it could make this
simple function fat.
P.S. Take into account that the user of it is only one so far, I would
like to hear a Mimi's opinion.

-- 
With Best Regards,
Andy Shevchenko

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

[Mimi Zohar]

On Mon, 2011-07-18 at 22:44 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 10:20 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> >> > 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.  :-)
> The key word "until now". But people will start to use anything which
> has public API, won't they?

Someone with more experience than me needs to responds.

> >  I'll update trusted/encrypted keys to check the return
> > code.
> Actually another question shall we add __must_check to the prototype or not?

Probably a good idea.

thanks,

Mimi

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

[Mimi Zohar]

On Tue, 2011-07-19 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 11:49 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > What about making it return the number of unprocessed bytes left instead?
> > Then the caller knows where the problem lies. And zero would mean success.
> If I remember correctly it used to be src as return value in some
> version of that patch. I don't know the details of that interim
> solution. My current opinion is to return boolean and make an
> additional parameter to return src value. However, it could make this
> simple function fat.
> P.S. Take into account that the user of it is only one so far, I would
> like to hear a Mimi's opinion.

Trusted/encrypted keys are not in a critical code path. They're used for
loading/storing key blobs from userspace. Once you change the API, short
circuiting out and adding an error return, from a trusted/encrypted key
perspective, it doesn't make a difference.

thanks,

Mimi

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

[Mimi Zohar]

(sorry for re-posting, but this doesn't seem to have been sent.)

On Tue, 2011-07-19 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 11:49 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > What about making it return the number of unprocessed bytes left instead?
> > Then the caller knows where the problem lies. And zero would mean success.
> If I remember correctly it used to be src as return value in some
> version of that patch. I don't know the details of that interim
> solution. My current opinion is to return boolean and make an
> additional parameter to return src value. However, it could make this
> simple function fat.
> P.S. Take into account that the user of it is only one so far, I would
> like to hear a Mimi's opinion.
> 

Trusted/encrypted keys are not in a critical code path. They're used for
loading/storing key blobs from userspace. Once you change the API, short
circuiting out and adding an error return, from a trusted/encrypted key
perspective, it doesn't make a difference.

thanks,

Mimi

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

[Mimi Zohar]

(sorry for re-posting, but this doesn't seem to have made it to the
lists.)

On Tue, 2011-07-19 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2011 at 11:49 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > What about making it return the number of unprocessed bytes left instead?
> > Then the caller knows where the problem lies. And zero would mean success.
> If I remember correctly it used to be src as return value in some
> version of that patch. I don't know the details of that interim
> solution. My current opinion is to return boolean and make an
> additional parameter to return src value. However, it could make this
> simple function fat.
> P.S. Take into account that the user of it is only one so far, I would
> like to hear a Mimi's opinion.
> 

Trusted/encrypted keys are not in a critical code path. They're used for
loading/storing key blobs from userspace.  From a trusted/encrypted key
perspective, it doesn't make much of a difference.

thanks,

Mimi