| Message ID | 201107182148.AGD21306.FOLtJVMSOOFQHF@I-love.SAKURA.ne.jp |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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/ >
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
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?
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
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.
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
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
(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
(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
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);