lkml.org 
[lkml]   [2011]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] Add error check to hex2bin().
    From
    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
    --
    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/

    \
     
     \ /
      Last update: 2011-07-18 20:59    [W:4.392 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site