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
