[bug report] UBI: Unsorted Block Images

[Dan Carpenter]

On Mon, Mar 31, 2025 at 08:43:55PM +0800, Zhihao Cheng wrote:
> 在 2025/3/31 17:46, Dan Carpenter 写道:
> Hi Dan,
> > 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
> > 
> [...]
> >     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?
> 
> Yes, leb 1 could be corrupted(eg. bad ec header), then the leb 1 won't be
> scanned during attaching, which means 'leb[1]' won't be initialized by
> process_lvol().
> > 
> >     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?
> 
> I guess that '#1' refers to leb 0 and '#2' refers to leb 1.
> > 
> >     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.
> 
> In theory, the 'NULL' won't be a return value, these cases are avoided by
> leb_corrupted checking branches.

No, that doesn't follow.  If leb[1] is NULL then we know that
leb_corrupted[1] is zero.  It's only set when leb[1] is non-NULL.

regards,
dan carpenter


> > 
> >     468          }
> >     469
> >     470  out_free:
> >     471          vfree(leb[0]);
> >     472          vfree(leb[1]);
> >     473          return ERR_PTR(err);
> >     474  }
> > 
> > 
> > regards,
> > dan carpenter
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> > .
> >