* [bug report] media: coda: jpeg: add CODA960 JPEG encoder support
@ 2021-04-23 11:44 Dan Carpenter
2021-04-23 12:51 ` Philipp Zabel
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-04-23 11:44 UTC (permalink / raw)
To: p.zabel; +Cc: linux-media
Hello Philipp Zabel,
The patch 96f6f62c4656: "media: coda: jpeg: add CODA960 JPEG encoder
support" from Dec 12, 2019, leads to the following static checker
warning:
drivers/media/platform/coda/coda-jpeg.c:621 coda9_jpeg_gen_enc_huff_tab()
warn: check that incremented offset 'k' is capped
drivers/media/platform/coda/coda-jpeg.c
582 static int coda9_jpeg_gen_enc_huff_tab(struct coda_ctx *ctx, int tab_num,
583 int *ehufsi, int *ehufco)
584 {
585 int i, j, k, lastk, si, code, maxsymbol;
586 const u8 *bits, *huffval;
587 struct {
588 int size[256];
589 int code[256];
590 } *huff;
591 static const unsigned char *huff_tabs[4] = {
592 luma_dc, luma_ac, chroma_dc, chroma_ac,
593 };
594 int ret = -EINVAL;
595
596 huff = kzalloc(sizeof(*huff), GFP_KERNEL);
huff->size[] is a 256 byte array of zero.
597 if (!huff)
598 return -ENOMEM;
599
600 bits = huff_tabs[tab_num];
601 huffval = huff_tabs[tab_num] + 16;
602
603 maxsymbol = tab_num & 1 ? 256 : 16;
604
605 /* Figure C.1 - Generation of table of Huffman code sizes */
606 k = 0;
607 for (i = 1; i <= 16; i++) {
608 j = bits[i - 1];
609 if (k + j > maxsymbol)
610 goto out;
611 while (j--)
612 huff->size[k++] = i;
^^^^^^^^^^^^^^^^^^^
We're filling potentially up to to maxsymbol (256) with i.
613 }
614 lastk = k;
615
616 /* Figure C.2 - Generation of table of Huffman codes */
617 k = 0;
618 code = 0;
619 si = huff->size[0];
620 while (k < lastk) {
621 while (huff->size[k] == si) {
^^^^^^^^^^^^^^^^^^^
I'm not sure I have understood this code, but I think maybe we do need
to if (k < lastk && huff->size[k] == si) {
622 huff->code[k++] = code;
623 code++;
624 }
625 if (code >= (1 << si))
626 goto out;
627 code <<= 1;
628 si++;
629 }
630
631 /* Figure C.3 - Ordering procedure for encoding procedure code tables */
632 for (k = 0; k < lastk; k++) {
633 i = huffval[k];
634 if (i >= maxsymbol || ehufsi[i])
635 goto out;
636 ehufco[i] = huff->code[k];
637 ehufsi[i] = huff->size[k];
638 }
639
640 ret = 0;
641 out:
642 kfree(huff);
643 return ret;
644 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] media: coda: jpeg: add CODA960 JPEG encoder support
2021-04-23 11:44 [bug report] media: coda: jpeg: add CODA960 JPEG encoder support Dan Carpenter
@ 2021-04-23 12:51 ` Philipp Zabel
0 siblings, 0 replies; 4+ messages in thread
From: Philipp Zabel @ 2021-04-23 12:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Hi Dan,
On Fri, 2021-04-23 at 14:44 +0300, Dan Carpenter wrote:
> Hello Philipp Zabel,
>
> The patch 96f6f62c4656: "media: coda: jpeg: add CODA960 JPEG encoder
> support" from Dec 12, 2019, leads to the following static checker
> warning:
>
> drivers/media/platform/coda/coda-jpeg.c:621 coda9_jpeg_gen_enc_huff_tab()
> warn: check that incremented offset 'k' is capped
Thank you for the report!
> drivers/media/platform/coda/coda-jpeg.c
> 582 static int coda9_jpeg_gen_enc_huff_tab(struct coda_ctx *ctx, int tab_num,
> 583 int *ehufsi, int *ehufco)
> 584 {
> 585 int i, j, k, lastk, si, code, maxsymbol;
> 586 const u8 *bits, *huffval;
> 587 struct {
> 588 int size[256];
> 589 int code[256];
> 590 } *huff;
> 591 static const unsigned char *huff_tabs[4] = {
> 592 luma_dc, luma_ac, chroma_dc, chroma_ac,
> 593 };
> 594 int ret = -EINVAL;
> 595
> 596 huff = kzalloc(sizeof(*huff), GFP_KERNEL);
>
> huff->size[] is a 256 byte array of zero.
int, not byte. It's does contain all zeros though, as does the following
huff->code[].
> 597 if (!huff)
> 598 return -ENOMEM;
> 599
> 600 bits = huff_tabs[tab_num];
> 601 huffval = huff_tabs[tab_num] + 16;
> 602
> 603 maxsymbol = tab_num & 1 ? 256 : 16;
> 604
> 605 /* Figure C.1 - Generation of table of Huffman code sizes */
> 606 k = 0;
> 607 for (i = 1; i <= 16; i++) {
> 608 j = bits[i - 1];
> 609 if (k + j > maxsymbol)
> 610 goto out;
> 611 while (j--)
> 612 huff->size[k++] = i;
> ^^^^^^^^^^^^^^^^^^^
> We're filling potentially up to to maxsymbol (256) with i.
That is correct. Assuming the sum of bits[0..15] is exactly 256, this
code will fill all 256 size[] entries.
In practice the four bits tables from huff_tabs only sum up to 162 at
most, but this may change if we added support for setting the encoder
Huffman tables from userspace.
> 613 }
> 614 lastk = k;
> 615
> 616 /* Figure C.2 - Generation of table of Huffman codes */
> 617 k = 0;
> 618 code = 0;
> 619 si = huff->size[0];
> 620 while (k < lastk) {
> 621 while (huff->size[k] == si) {
> ^^^^^^^^^^^^^^^^^^^
> I'm not sure I have understood this code, but I think maybe we do need
> to if (k < lastk && huff->size[k] == si) {
So this could indeed overflow size[] in theory.
It would still terminate at &size[256] or &code[0], as code[0] happens
to always contain 0, but your suggestion is correct.
> 622 huff->code[k++] = code;
> 623 code++;
> 624 }
> 625 if (code >= (1 << si))
> 626 goto out;
> 627 code <<= 1;
> 628 si++;
> 629 }
regards
Philipp
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] media: coda: jpeg: add CODA960 JPEG encoder support
@ 2022-06-15 8:20 Dan Carpenter
2022-06-15 12:24 ` Philipp Zabel
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-06-15 8:20 UTC (permalink / raw)
To: p.zabel; +Cc: linux-media
Hello Philipp Zabel,
The patch 96f6f62c4656: "media: coda: jpeg: add CODA960 JPEG encoder
support" from Dec 12, 2019, leads to the following Smatch static
checker warning:
drivers/media/platform/chips-media/coda-jpeg.c:622 coda9_jpeg_gen_enc_huff_tab()
warn: check that incremented offset 'k' is capped
drivers/media/platform/chips-media/coda-jpeg.c
583 static int coda9_jpeg_gen_enc_huff_tab(struct coda_ctx *ctx, int tab_num,
584 int *ehufsi, int *ehufco)
585 {
586 int i, j, k, lastk, si, code, maxsymbol;
587 const u8 *bits, *huffval;
588 struct {
589 int size[256];
590 int code[256];
591 } *huff;
592 static const unsigned char *huff_tabs[4] = {
593 luma_dc, luma_ac, chroma_dc, chroma_ac,
594 };
595 int ret = -EINVAL;
596
597 huff = kzalloc(sizeof(*huff), GFP_KERNEL);
598 if (!huff)
599 return -ENOMEM;
600
601 bits = huff_tabs[tab_num];
602 huffval = huff_tabs[tab_num] + 16;
603
604 maxsymbol = tab_num & 1 ? 256 : 16;
605
606 /* Figure C.1 - Generation of table of Huffman code sizes */
607 k = 0;
608 for (i = 1; i <= 16; i++) {
609 j = bits[i - 1];
610 if (k + j > maxsymbol)
611 goto out;
612 while (j--)
613 huff->size[k++] = i;
614 }
615 lastk = k;
616
617 /* Figure C.2 - Generation of table of Huffman codes */
618 k = 0;
619 code = 0;
620 si = huff->size[0];
621 while (k < lastk) {
^^^^^^^^^
Here we know that k is valid.
--> 622 while (huff->size[k] == si) {
623 huff->code[k++] = code;
But this loop iterates through k without checking if k is still valid.
How do we know that the huff->size[k] check won't read beyond the end
of the loop? Presumably it won't go far beyond the end before it hits
something which is != si.
624 code++;
625 }
626 if (code >= (1 << si))
627 goto out;
628 code <<= 1;
629 si++;
630 }
631
632 /* Figure C.3 - Ordering procedure for encoding procedure code tables */
633 for (k = 0; k < lastk; k++) {
634 i = huffval[k];
635 if (i >= maxsymbol || ehufsi[i])
636 goto out;
637 ehufco[i] = huff->code[k];
638 ehufsi[i] = huff->size[k];
639 }
640
641 ret = 0;
642 out:
643 kfree(huff);
644 return ret;
645 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] media: coda: jpeg: add CODA960 JPEG encoder support
2022-06-15 8:20 Dan Carpenter
@ 2022-06-15 12:24 ` Philipp Zabel
0 siblings, 0 replies; 4+ messages in thread
From: Philipp Zabel @ 2022-06-15 12:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Hi Dan,
On Mi, 2022-06-15 at 11:20 +0300, Dan Carpenter wrote:
> Hello Philipp Zabel,
>
> The patch 96f6f62c4656: "media: coda: jpeg: add CODA960 JPEG encoder
> support" from Dec 12, 2019, leads to the following Smatch static
> checker warning:
>
> drivers/media/platform/chips-media/coda-jpeg.c:622 coda9_jpeg_gen_enc_huff_tab()
> warn: check that incremented offset 'k' is capped
Thank you for the report. I think k is currently guaranteed to be
capped, but this depends on the data stored in the luma/chroma_dc/ac
tables.
Which smatch version is this? I didn't get this warning with commit
1a0af070bd4d ("select_type: new check for propagating negatives to u32
to s64").
> drivers/media/platform/chips-media/coda-jpeg.c
> 583 static int coda9_jpeg_gen_enc_huff_tab(struct coda_ctx *ctx, int tab_num,
> 584 int *ehufsi, int *ehufco)
> 585 {
> 586 int i, j, k, lastk, si, code, maxsymbol;
> 587 const u8 *bits, *huffval;
> 588 struct {
> 589 int size[256];
> 590 int code[256];
> 591 } *huff;
> 592 static const unsigned char *huff_tabs[4] = {
> 593 luma_dc, luma_ac, chroma_dc, chroma_ac,
We use the hardcoded Huffman bit/value tables from from JPEG ITU-T.81,
for example luma_ac:
0x00, 0x02, 0x01, 0x03, 0x03, 0x02, 0x04, 0x03,
0x05, 0x05, 0x04, 0x04, 0x00, 0x00, 0x01, 0x7d,
...
The sum of the first 16 values in these tables is either 12 for the _dc
tables or 162 for the _ac tables.
> 594 };
> 595 int ret = -EINVAL;
> 596
> 597 huff = kzalloc(sizeof(*huff), GFP_KERNEL);
> 598 if (!huff)
> 599 return -ENOMEM;
> 600
> 601 bits = huff_tabs[tab_num];
The sum of all 16 values in the bits table is at most 162.
> 602 huffval = huff_tabs[tab_num] + 16;
> 603
> 604 maxsymbol = tab_num & 1 ? 256 : 16;
> 605
> 606 /* Figure C.1 - Generation of table of Huffman code sizes */
> 607 k = 0;
> 608 for (i = 1; i <= 16; i++) {
> 609 j = bits[i - 1];
> 610 if (k + j > maxsymbol)
> 611 goto out;
> 612 while (j--)
> 613 huff->size[k++] = i;
Here we can increment k only to the sum of bits[0..16], 162 at most.
This leaves huff->size[162..255] set to 0.
huff->size[0] is set to 1.
> 614 }
> 615 lastk = k;
> 616
> 617 /* Figure C.2 - Generation of table of Huffman codes */
> 618 k = 0;
> 619 code = 0;
> 620 si = huff->size[0];
> 621 while (k < lastk) {
> ^^^^^^^^^
> Here we know that k is valid.
>
> --> 622 while (huff->size[k] == si) {
This iteration stops at k == 162 at the latest, since si is at least 1
and huff->size[162] == 0.
> 623 huff->code[k++] = code;
>
> But this loop iterates through k without checking if k is still valid.
> How do we know that the huff->size[k] check won't read beyond the end
> of the loop?
See above.
regards
Philipp
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-15 12:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 11:44 [bug report] media: coda: jpeg: add CODA960 JPEG encoder support Dan Carpenter
2021-04-23 12:51 ` Philipp Zabel
2022-06-15 8:20 Dan Carpenter
2022-06-15 12:24 ` Philipp Zabel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.