* re: [media] gspca - topro: New subdriver for Topro webcams
@ 2014-01-30 12:14 Dan Carpenter
2014-01-30 19:01 ` Jean-Francois Moine
2014-02-23 22:04 ` Hans de Goede
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2014-01-30 12:14 UTC (permalink / raw)
To: moinejf; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [media] gspca - topro: New subdriver for Topro webcams
2014-01-30 12:14 [media] gspca - topro: New subdriver for Topro webcams Dan Carpenter
@ 2014-01-30 19:01 ` Jean-Francois Moine
2014-02-23 22:04 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Jean-Francois Moine @ 2014-01-30 19:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
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/
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [media] gspca - topro: New subdriver for Topro webcams
2014-01-30 12:14 [media] gspca - topro: New subdriver for Topro webcams Dan Carpenter
2014-01-30 19:01 ` Jean-Francois Moine
@ 2014-02-23 22:04 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2014-02-23 22:04 UTC (permalink / raw)
To: Dan Carpenter, moinejf; +Cc: linux-media
Hi,
On 01/30/2014 01:14 PM, Dan Carpenter 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?
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-23 22:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-30 12:14 [media] gspca - topro: New subdriver for Topro webcams Dan Carpenter
2014-01-30 19:01 ` Jean-Francois Moine
2014-02-23 22:04 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox