Re: [media] gspca - topro: New subdriver for Topro webcams

[Jean-Francois Moine <moinejf@free.fr>]

On Thu, 30 Jan 2014 15:14:09 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> 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  }

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/