gspca - topro: New subdriver for Topro webcams

Hello Jean-François Moine,

The patch 8f12b1ab2fac: "[media] gspca - topro: New subdriver for Topro webcams" from Sep 22, 2011, leads to the following static checker warning:

	drivers/media/usb/gspca/topro.c:4642
	sd_pkt_scan() warn: check 'data[]' for negative offsets s32min"

drivers/media/usb/gspca/topro.c
4632 data++;

Should there be an "if (len < 8) return;" here?

4633 len--;
4634 if (*data == 0xff && data[1] == 0xd8) {
4635 /*fixme: there may be information in the 4 high bits*/
4636 if ((data[6] & 0x0f) != sd->quality)
4637 set_dqt(gspca_dev, data[6] & 0x0f);
4638 gspca_frame_add(gspca_dev, FIRST_PACKET,
4639 sd->jpeg_hdr, JPEG_HDR_SZ);
4640 gspca_frame_add(gspca_dev, INTER_PACKET,
4641 data + 7, len - 7);
4642 } else if (data[len - 2] == 0xff && data[len - 1] == 0xd9) {
4643 gspca_frame_add(gspca_dev, LAST_PACKET,
4644 data, len);
4645 } else {
4646 gspca_frame_add(gspca_dev, INTER_PACKET,
4647 data, len);
4648 }
4649 return;
4650 }
4651
4652 switch (*data) {
4653 case 0x55:
4654 gspca_frame_add(gspca_dev, LAST_PACKET, data, 0);
4655
4656 if (len < 8

                            ^^^^^^^

The same as there is here.

  4657                   || data[1] != 0xff || data[2] != 0xd8
  4658                   || data[3] != 0xff || data[4] != 0xfe) {
  4659  
  4660                          /* Have only seen this with corrupt frames */
  4661                          gspca_dev->last_packet_type = DISCARD_PACKET;
  4662                          return;

4663 }
4664 if (data[7] != sd->quality)
4665 set_dqt(gspca_dev, data[7]);
4666 gspca_frame_add(gspca_dev, FIRST_PACKET,
4667 sd->jpeg_hdr, JPEG_HDR_SZ);
4668 gspca_frame_add(gspca_dev, INTER_PACKET,
4669 data + 8, len - 8);
4670 break;
4671 case 0xaa:
4672 gspca_dev->last_packet_type = DISCARD_PACKET;
4673 break;
4674 case 0xcc:

I suppose we could add a "if (len < 1)" here as well.

  4675                  if (data[1] != 0xff || data[2] != 0xd8)
  4676                          gspca_frame_add(gspca_dev, INTER_PACKET,
  4677                                          data + 1, len - 1);
  4678                  else
  4679                          gspca_dev->last_packet_type = DISCARD_PACKET;
  4680                  break;
  4681          }
  4682  }

regards,
dan carpenter

AFAIR, there should be no bug because:

- for the BRIDGE_TP6810
- no, there shoud not be a "if (len < 8) return;" at the place you put it: the end of image may be indicated by just 0x5a, 0xff, 0xd9.

- when the first byte is 0x5a, there are always at least 3 bytes.

- when the 2nd and 3rd bytes are 0xff, 0xd8, there are always at least 8 bytes. I never saw corrupt packets.

- for the BRIDGE_TP6800
- when the first byte is 0x55 (start of image), I saw some corrupt packets with less than 8 bytes. So, the test is there.

- when the first byte is 0xcc, it is an intermediate packet, so it always contains some data. I never saw such packets reduced to less than 3 bytes.

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Hi,

Thanks for the report, there were indeed several missing length checks in the packet parsing code in topro.c

I've added a patch fixing this to my gspca tree for 3.15 .

Regards,

Hans