Re: [bug report] media: coda: jpeg: add CODA960 JPEG encoder support

[Philipp Zabel <p.zabel@pengutronix.de>]

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