Patchwork Add error check to hex2bin().

login
register
mail settings
Submitter Tetsuo Handa
Date July 18, 2011, 12:48 p.m.
Message ID <201107182148.AGD21306.FOLtJVMSOOFQHF@I-love.SAKURA.ne.jp>
Download mbox | patch
Permalink /patch/105258/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Tetsuo Handa - July 18, 2011, 12:48 p.m.
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>
---


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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar - July 18, 2011, 6:03 p.m.
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
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko - July 18, 2011, 6:57 p.m.
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/
>
Mimi Zohar - July 18, 2011, 7:20 p.m.
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;
> >


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko - July 18, 2011, 7:44 p.m.
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?
Geert Uytterhoeven - July 18, 2011, 8:49 p.m.
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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko - July 18, 2011, 9:18 p.m.
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.
Mimi Zohar - July 18, 2011, 9:43 p.m.
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar - July 18, 2011, 9:59 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar - July 19, 2011, 1 a.m.
(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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar - July 19, 2011, 10:52 a.m.
(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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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