[bug report] UBI: Unsorted Block Images
Dan Carpenter
dan.carpenter at linaro.org
Mon Mar 31 02:46:52 PDT 2025
Hello Artem B. Bityutskiy,
Commit 801c135ce73d ("UBI: Unsorted Block Images") from Jun 27, 2006
(linux-next), leads to the following Smatch static checker warning:
drivers/mtd/ubi/vtbl.c:844 ubi_read_volume_table()
warn: 'ubi->vtbl' can also be NULL
drivers/mtd/ubi/vtbl.c
826 if (av->leb_count > UBI_LAYOUT_VOLUME_EBS) {
827 /* This must not happen with proper UBI images */
828 ubi_err(ubi, "too many LEBs (%d) in layout volume",
829 av->leb_count);
830 return -EINVAL;
831 }
832
833 ubi->vtbl = process_lvol(ubi, ai, av);
^^^^^^^^^^^^^^^^^^^^^^^^
If process_lvol() returns a NULL then it leads to a crash.
834 if (IS_ERR(ubi->vtbl))
835 return PTR_ERR(ubi->vtbl);
836 }
The process_lvol() function looks like this:
drivers/mtd/ubi/vtbl.c
351 /**
352 * process_lvol - process the layout volume.
353 * @ubi: UBI device description object
354 * @ai: attaching information
355 * @av: layout volume attaching information
356 *
357 * This function is responsible for reading the layout volume, ensuring it is
358 * not corrupted, and recovering from corruptions if needed. Returns volume
359 * table in case of success and a negative error code in case of failure.
As I read it, this comment says that it returns error pointers or valid.
360 */
361 static struct ubi_vtbl_record *process_lvol(struct ubi_device *ubi,
362 struct ubi_attach_info *ai,
363 struct ubi_ainf_volume *av)
364 {
365 int err;
366 struct rb_node *rb;
367 struct ubi_ainf_peb *aeb;
368 struct ubi_vtbl_record *leb[UBI_LAYOUT_VOLUME_EBS] = { NULL, NULL };
369 int leb_corrupted[UBI_LAYOUT_VOLUME_EBS] = {1, 1};
370
371 /*
372 * UBI goes through the following steps when it changes the layout
373 * volume:
374 * a. erase LEB 0;
375 * b. write new data to LEB 0;
376 * c. erase LEB 1;
377 * d. write new data to LEB 1.
378 *
379 * Before the change, both LEBs contain the same data.
380 *
381 * Due to unclean reboots, the contents of LEB 0 may be lost, but there
382 * should LEB 1. So it is OK if LEB 0 is corrupted while LEB 1 is not.
383 * Similarly, LEB 1 may be lost, but there should be LEB 0. And
384 * finally, unclean reboots may result in a situation when neither LEB
385 * 0 nor LEB 1 are corrupted, but they are different. In this case, LEB
386 * 0 contains more recent information.
387 *
388 * So the plan is to first check LEB 0. Then
389 * a. if LEB 0 is OK, it must be containing the most recent data; then
390 * we compare it with LEB 1, and if they are different, we copy LEB
391 * 0 to LEB 1;
392 * b. if LEB 0 is corrupted, but LEB 1 has to be OK, and we copy LEB 1
393 * to LEB 0.
394 */
395
396 dbg_gen("check layout volume");
397
398 /* Read both LEB 0 and LEB 1 into memory */
399 ubi_rb_for_each_entry(rb, aeb, &av->root, u.rb) {
400 leb[aeb->lnum] = vzalloc(ubi->vtbl_size);
401 if (!leb[aeb->lnum]) {
402 err = -ENOMEM;
403 goto out_free;
404 }
405
406 err = ubi_io_read_data(ubi, leb[aeb->lnum], aeb->pnum, 0,
407 ubi->vtbl_size);
408 if (err == UBI_IO_BITFLIPS || mtd_is_eccerr(err))
409 /*
410 * Scrub the PEB later. Note, -EBADMSG indicates an
411 * uncorrectable ECC error, but we have our own CRC and
412 * the data will be checked later. If the data is OK,
413 * the PEB will be scrubbed (because we set
414 * aeb->scrub). If the data is not OK, the contents of
415 * the PEB will be recovered from the second copy, and
416 * aeb->scrub will be cleared in
417 * 'ubi_add_to_av()'.
418 */
419 aeb->scrub = 1;
420 else if (err)
421 goto out_free;
422 }
423
424 err = -EINVAL;
425 if (leb[0]) {
426 leb_corrupted[0] = vtbl_check(ubi, leb[0]);
427 if (leb_corrupted[0] < 0)
428 goto out_free;
429 }
430
431 if (!leb_corrupted[0]) {
432 /* LEB 0 is OK */
433 if (leb[1])
434 leb_corrupted[1] = memcmp(leb[0], leb[1],
435 ubi->vtbl_size);
436 if (leb_corrupted[1]) {
437 ubi_warn(ubi, "volume table copy #2 is corrupted");
438 err = create_vtbl(ubi, ai, 1, leb[0]);
439 if (err)
440 goto out_free;
441 ubi_msg(ubi, "volume table was restored");
442 }
443
444 /* Both LEB 1 and LEB 2 are OK and consistent */
445 vfree(leb[1]);
446 return leb[0];
447 } else {
448 /* LEB 0 is corrupted or does not exist */
449 if (leb[1]) {
^^^^^^
Is this NULL check required?
450 leb_corrupted[1] = vtbl_check(ubi, leb[1]);
451 if (leb_corrupted[1] < 0)
452 goto out_free;
453 }
454 if (leb_corrupted[1]) {
455 /* Both LEB 0 and LEB 1 are corrupted */
456 ubi_err(ubi, "both volume tables are corrupted");
457 goto out_free;
458 }
459
460 ubi_warn(ubi, "volume table copy #1 is corrupted");
^^
This should say #0 I think?
461 err = create_vtbl(ubi, ai, 0, leb[1]);
^^^^^^
462 if (err)
463 goto out_free;
464 ubi_msg(ubi, "volume table was restored");
465
466 vfree(leb[0]);
467 return leb[1];
^^^^^^
The NULL check on line 449 makes Smatch think this can be NULL.
468 }
469
470 out_free:
471 vfree(leb[0]);
472 vfree(leb[1]);
473 return ERR_PTR(err);
474 }
regards,
dan carpenter
More information about the linux-mtd
mailing list