[bug report] UBI: Unsorted Block Images

[Zhihao Cheng]

在 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?

> 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/
>>> .
>>>
> .
>