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.
--Sean
[1] http://linux-mtd.infradead.org/nand-data/nanddata.html