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
