Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 11301 bytes --]

On Tue, Jan 09, 2024 at 12:26:13AM -0500, Sean Anderson wrote:
> Comments on NAND stuff only.
> 
> On 1/8/24 12:45, Tom Rini wrote:
> > ________________________________________________________________________________________________________
> > *** CID 477216:    (BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
> > 3915
> > 3916            /*
> > 3917             * pages_per_block and blocks_per_lun may not be a
> > power-of-2 size
> > 3918             * (don't ask me who thought of this...). MTD assumes that these
> > 3919             * dimensions will be power-of-2, so just truncate the
> > remaining area.
> > 3920             */
> > > > >      CID 477216:    (BAD_SHIFT)
> > > > >      In expression "1 << generic_fls((__u32)(__le32)p->pages_per_block) - 1", shifting by a negative amount has undefined behavior.  The shift amount, "generic_fls((__u32)(__le32)p->pages_per_block) - 1", is -1.
> > 3921            mtd->erasesize = 1 <<
> > (fls(le32_to_cpu(p->pages_per_block)) - 1);
> > 3922            mtd->erasesize *= mtd->writesize;
> > 3923
> > 3924            mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> > 3925
> > 3926            /* See erasesize comment */
> > /drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
> > 3921            mtd->erasesize = 1 <<
> > (fls(le32_to_cpu(p->pages_per_block)) - 1);
> > 3922            mtd->erasesize *= mtd->writesize;
> > 3923
> > 3924            mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> > 3925
> > 3926            /* See erasesize comment */
> > > > >      CID 477216:    (BAD_SHIFT)
> > > > >      In expression "1 << generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", shifting by a negative amount has undefined behavior.  The shift amount, "generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", is -1.
> > 3927            chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
> > 3928            chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
> > 3929            chip->bits_per_cell = p->bits_per_cell;
> > 3930
> > 3931            if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
> > 3932                    chip->options |= NAND_BUSWIDTH_16;
> 
> Yeah, this looks like a bug.
> 
> > ** CID 477215:  Control flow issues  (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477215:  Control flow issues  (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
> > 4972                            pr_warn("No ECC functions supplied;
> > hardware ECC not possible\n");
> > 4973                            BUG();
> > 4974                    }
> > 4975                    if (!ecc->read_page)
> > 4976                            ecc->read_page = nand_read_page_hwecc_oob_first;
> > 4977
> > > > >      CID 477215:  Control flow issues  (MISSING_BREAK)
> > > > >      The case for value "NAND_ECC_HW" is not terminated by a "break" statement.
> > 4978            case NAND_ECC_HW:
> > 4979                    /* Use standard hwecc read page function? */
> > 4980                    if (!ecc->read_page)
> > 4981                            ecc->read_page = nand_read_page_hwecc;
> > 4982                    if (!ecc->write_page)
> > 4983                            ecc->write_page = nand_write_page_hwecc;
> 
> I think we just need a fallthrough comment here.
> 
> > ** CID 477214:  Integer handling issues  (BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477214:  Integer handling issues  (BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
> > 4391
> > 4392            nand_decode_bbm_options(mtd, chip);
> > 4393
> > 4394            /* Calculate the address shift from the page size */
> > 4395            chip->page_shift = ffs(mtd->writesize) - 1;
> > 4396            /* Convert chipsize to number of pages per chip -1 */
> > > > >      CID 477214:  Integer handling issues  (BAD_SHIFT)
> > > > >      In expression "chip->chipsize >> chip->page_shift", shifting by a negative amount has undefined behavior.  The shift amount, "chip->page_shift", is -1.
> > 4397            chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
> > 4398
> > 4399            chip->bbt_erase_shift = chip->phys_erase_shift =
> > 4400                    ffs(mtd->erasesize) - 1;
> > 4401            if (chip->chipsize & 0xffffffff)
> > 4402                    chip->chip_shift = ffs((unsigned)chip->chipsize) - 1;
> 
> Buggy, but only when writesize is 0 (which is a bigger bug in the nand chip).
> 
> > ** CID 477213:  Security best practices violations  (DC.WEAK_CRYPTO)
> > /test/dm/nand.c: 67 in dm_test_nand()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477213:  Security best practices violations  (DC.WEAK_CRYPTO)
> > /test/dm/nand.c: 67 in dm_test_nand()
> > 61      ops.ooblen = mtd->oobsize;
> > 62      ut_assertok(mtd_read_oob(mtd, mtd->erasesize, &ops));
> > 63      ut_asserteq(0, oob[mtd_to_nand(mtd)->badblockpos]);
> > 64
> > 65      /* Generate some data and write it */
> > 66      for (i = 0; i < size / sizeof(int); i++)
> > > > >      CID 477213:  Security best practices violations  (DC.WEAK_CRYPTO)
> > > > >      "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
> > 67              gold[i] = rand();
> > 68      ut_assertok(nand_write_skip_bad(mtd, off, &length, NULL, U64_MAX,
> > 69                                      (void *)gold, 0));
> > 70      ut_asserteq(size, length);
> > 71
> > 72      /* Verify */
> 
> Not a bug.
> 
> > ** CID 477211:  API usage errors  (ALLOC_FREE_MISMATCH)
> > /drivers/mtd/nand/raw/nand_bbt.c: 1133 in nand_scan_bbt()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477211:  API usage errors  (ALLOC_FREE_MISMATCH)
> > /drivers/mtd/nand/raw/nand_bbt.c: 1133 in nand_scan_bbt()
> > 1127
> > 1128            /* Prevent the bbt regions from erasing / writing */
> > 1129            mark_bbt_region(mtd, td);
> > 1130            if (md)
> > 1131                    mark_bbt_region(mtd, md);
> > 1132
> > > > >      CID 477211:  API usage errors  (ALLOC_FREE_MISMATCH)
> > > > >      Calling "vfree" frees "buf" using "vfree" but it should have been freed using "kfree". [Note: The source code implementation of the function has been overridden by a builtin model.]
> > 1133            vfree(buf);
> > 1134            return 0;
> > 1135
> > 1136     err:
> > 1137            kfree(this->bbt);
> > 1138            this->bbt = NULL;
> 
> Not a bug, since these both call free().
> 
> > ** CID 477210:  Security best practices violations  (DC.WEAK_CRYPTO)
> > /drivers/mtd/nand/raw/sand_nand.c: 199 in sand_nand_read()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477210:  Security best practices violations  (DC.WEAK_CRYPTO)
> > /drivers/mtd/nand/raw/sand_nand.c: 199 in sand_nand_read()
> > 193             chip->tmp_dirty = true;
> > 194             for (i = 0; i < chip->err_steps; i++) {
> > 195                     u32 bit_errors = chip->err_count;
> > 196                     unsigned int j = chip->err_step_bits + chip->ecc_bits;
> > 197
> > 198                     while (bit_errors) {
> > > > >      CID 477210:  Security best practices violations  (DC.WEAK_CRYPTO)
> > > > >      "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
> > 199                             unsigned int u = rand();
> > 200                             float quot = 1ULL << 32;
> > 201
> > 202                             do {
> > 203                                     quot *= j - bit_errors;
> > 204                                     quot /= j;
> 
> Not a bug.
> 
> > ** CID 477207:  Control flow issues  (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4969 in nand_scan_tail()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477207:  Control flow issues  (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4969 in nand_scan_tail()
> > 4963            /*
> > 4964             * Check ECC mode, default to software if
> > 3byte/512byte hardware ECC is
> > 4965             * selected and we have 256 byte pagesize fallback to
> > software ECC
> > 4966             */
> > 4967
> > 4968            switch (ecc->mode) {
> > > > >      CID 477207:  Control flow issues  (MISSING_BREAK)
> > > > >      The case for value "NAND_ECC_HW_OOB_FIRST" is not terminated by a "break" statement.
> > 4969            case NAND_ECC_HW_OOB_FIRST:
> > 4970                    /* Similar to NAND_ECC_HW, but a separate
> > read_page handle */
> > 4971                    if (!ecc->calculate || !ecc->correct || !ecc->hwctl) {
> > 4972                            pr_warn("No ECC functions supplied;
> > hardware ECC not possible\n");
> > 4973                            BUG();
> > 4974                    }
> 
> need a fallthrough comment
> 
> > ** CID 477205:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > /cmd/mtd.c: 88 in mtd_dump_device_buf()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 477205:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > /cmd/mtd.c: 88 in mtd_dump_device_buf()
> > 82                      printf("\nDump %d data bytes from 0x%08llx:\n",
> > 83                             mtd->writesize, start_off + data_off);
> > 84                      mtd_dump_buf(&buf[data_off],
> > 85                                   mtd->writesize, start_off + data_off);
> > 86
> > 87                      if (woob) {
> > > > >      CID 477205:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > >      Potentially overflowing expression "page * mtd->oobsize" with type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "u64" (64 bits, unsigned).
> > 88                              u64 oob_off = page * mtd->oobsize;
> > 89
> > 90                              printf("Dump %d OOB bytes from page at
> > 0x%08llx:\n",
> > 91                                     mtd->oobsize, start_off + data_off);
> > 92                              mtd_dump_buf(&buf[len + oob_off],
> > 93                                           mtd->oobsize, 0);
> 
> In the Linux MTD list [1], the largest this can be is 0xe0000000 for MT29F512G08CUCAB. That's worryingly
> close to overflow, so I'd say this is a bug.

Thanks, I've updated the not a bug ones in the dashboard.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]