Re: [dpdk-dev] [PATCH v1 2/3] net/hyperv: implement core functionality

[Adrien Mazarguil]

On Mon, Dec 18, 2017 at 03:59:46PM -0800, Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 17:46:23 +0100
> Adrien Mazarguil <adrien.mazarg...@6wind.com> wrote:
> 
> > +static int
> > +ether_addr_from_str(struct ether_addr *eth_addr, const char *str)
> > +{
> > +   static const uint8_t conv[0x100] = {
> > +           ['0'] = 0x80, ['1'] = 0x81, ['2'] = 0x82, ['3'] = 0x83,
> > +           ['4'] = 0x84, ['5'] = 0x85, ['6'] = 0x86, ['7'] = 0x87,
> > +           ['8'] = 0x88, ['9'] = 0x89, ['a'] = 0x8a, ['b'] = 0x8b,
> > +           ['c'] = 0x8c, ['d'] = 0x8d, ['e'] = 0x8e, ['f'] = 0x8f,
> > +           ['A'] = 0x8a, ['B'] = 0x8b, ['C'] = 0x8c, ['D'] = 0x8d,
> > +           ['E'] = 0x8e, ['F'] = 0x8f, [':'] = 0x40, ['-'] = 0x40,
> > +           ['\0'] = 0x60,
> > +   };
> > +   uint64_t addr = 0;
> > +   uint64_t buf = 0;
> > +   unsigned int i = 0;
> > +   unsigned int n = 0;
> > +   uint8_t tmp;
> > +
> > +   do {
> > +           tmp = conv[(int)*(str++)];
> 
> Cast to int will cause out of bounds reference on non-ascii strings.
> The parser will get confused by:
>     001:aa:bb:cc:dd:ee:ff or invalid strings.

Nice catch! I added the (int) cast to shut up a GCC complaint about using
char as index type. The proper fix taking care of integer conversion and
array bounds safety check should read:

 tmp = conv[*str++ & 0xffu];

> Why not use sscanf which would be safer in this case.

Right, this is indeed the obvious implementation, however not only the fixed
MAC-48 format is not the most convenient to use for user input (somewhat
like forcing them to enter fully expanded IPv6 addresses every time),
sscanf() also ignores leading white spaces and successfully parses weird
expressions like "  -42:    0x66:   0af: 0: 44:-6", which I think is a
problem.

> /**
>  * Parse 48bits Ethernet address in pattern xx:xx:xx:xx:xx:xx.
>  *
>  * @param eth_addr
>  *   A pointer to a ether_addr structure.
>  * @param str
>  *   A pointer to string contains the formatted MAC address.
>  * @return
>  *   0 if the address is valid
>  *  -EINVAL if address is not formatted properly
>  */
> static inline int
> ether_parse_addr(struct ether_addr *eth_addr, const char *str)
> {
>       int n;
>       
>       n = sscanf(str, 
>                  "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
>                  &eth_addr->addr_bytes[0],
>                  &eth_addr->addr_bytes[1],
>                  &eth_addr->addr_bytes[2],
>                  &eth_addr->addr_bytes[3],
>                  &eth_addr->addr_bytes[4],
>                  &eth_addr->addr_bytes[5]);
>       return (n == ETHER_ADDR_LEN) ? 0 : -EINVAL;
> }
> 
> > +           if (!tmp)
> > +                   return -EINVAL;
> > +           if (tmp & 0x40) {
> > +                   i += (i & 1) + (!i << 1);
> > +                   addr = (addr << (i << 2)) | buf;
> > +                   n += i;
> > +                   buf = 0;
> > +                   i = 0;
> > +           } else {
> > +                   buf = (buf << 4) | (tmp & 0xf);
> > +                   ++i;
> > +           }
> > +   } while (!(tmp & 0x20));
> > +   if (n > 12)
> > +           return -EINVAL;
> > +   i = RTE_DIM(eth_addr->addr_bytes);
> > +   while (i) {
> > +           eth_addr->addr_bytes[--i] = addr & 0xff;
> > +           addr >>= 8;
> > +   }
> > +   return 0;
> > +}
> > +

-- 
Adrien Mazarguil
6WIND