re: net/usb: Add Samsung Kalmia driver for Samsung GT-B3730

re: net/usb: Add Samsung Kalmia driver for Samsung GT-B3730

[Dan Carpenter]

I started writing a Smatch check to look for:

	skb->len - foo

where foo might be more than skb->len.  I don't know this code well
enough to say what the rules are exactly.  I chose a fairly suspect
bit of code to use as an example to get some feedback.

Hello Marius B. Kotsbak,

The patch d40261236e8e: "net/usb: Add Samsung Kalmia driver for
Samsung GT-B3730" from Jun 12, 2011, leads to the following static
checker warning:

	drivers/net/usb/kalmia.c:281 kalmia_rx_fixup()
	warn: this assumes skb->len is at least 12 bytes

drivers/net/usb/kalmia.c
   245          /* incomplete header? */
   246          if (skb->len < KALMIA_HEADER_LENGTH)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We verify that it is at least 6.

   247                  return 0;
   248  
   249          do {

This is a do while (skb->len) loop.  Shouldn't the check for invalid
skb->len be inside the loop?

   250                  struct sk_buff *skb2 = NULL;
   251                  u8 *header_start;
   252                  u16 usb_packet_length, ether_packet_length;
   253                  int is_last;
   254  
   255                  header_start = skb->data;
   256  
   257                  if (unlikely(header_start[0] != 0x57 || header_start[1] != 0x44)) {
   258                          if (!memcmp(header_start, EXPECTED_UNKNOWN_HEADER_1,
   259                                  sizeof(EXPECTED_UNKNOWN_HEADER_1)) || !memcmp(
   260                                  header_start, EXPECTED_UNKNOWN_HEADER_2,
   261                                  sizeof(EXPECTED_UNKNOWN_HEADER_2))) {
   262                                  netdev_dbg(dev->net,
   263                                          "Received expected unknown frame header: %6phC. Package length: %i\n",
   264                                          header_start,
   265                                          skb->len - KALMIA_HEADER_LENGTH);
   266                          }
   267                          else {
   268                                  netdev_err(dev->net,
   269                                          "Received unknown frame header: %6phC. Package length: %i\n",
   270                                          header_start,
   271                                          skb->len - KALMIA_HEADER_LENGTH);
   272                                  return 0;
   273                          }
   274                  }
   275                  else
   276                          netdev_dbg(dev->net,
   277                                  "Received header: %6phC. Package length: %i\n",
   278                                  header_start, skb->len - KALMIA_HEADER_LENGTH);
   279  
   280                  /* subtract start header and end header */
   281                  usb_packet_length = skb->len - (2 * KALMIA_HEADER_LENGTH);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We could end up with usb_packet_length is a negative number truncated
to a largish u16.  Positive obviously.

   282                  ether_packet_length = get_unaligned_le16(&header_start[2]);
   283                  skb_pull(skb, KALMIA_HEADER_LENGTH);
   284  
   285                  /* Some small packets misses end marker */
   286                  if (usb_packet_length < ether_packet_length) {

This condition is not true because usb_packet_length is largish.

   287                          ether_packet_length = usb_packet_length
   288                                  + KALMIA_HEADER_LENGTH;
   289                          is_last = true;
   290                  }
   291                  else {
   292                          netdev_dbg(dev->net, "Correct package length #%i", i
   293                                  + 1);
   294  

regards,
dan carpenter