[bug report] UBI: Unsorted Block Images

[Dan Carpenter]

On Tue, Apr 01, 2025 at 02:46:35PM +0800, Zhihao Cheng wrote:
> 在 2025/3/31 23:14, 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.
> > 
> 
> Hi Dan. The leb_corrupted array is initialized as '1' in line 369:
> 369         int leb_corrupted[UBI_LAYOUT_VOLUME_EBS] = {1, 1};
> 
> If leb[1] is NULL. the leb_corrupted[1] is non-zero, then the function will
> exit in following branch:
>  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                 }
> 
> Am i missing something?
> 

Sorry, yeah.  I got mixed up with the line before.  I apologize for that.

regards,
dan carpenter