[bug report] UBI: Unsorted Block Images

[Zhihao Cheng]

在 2025/4/1 16:11, 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.
> 

It's okay，anyone could read the code wrong. :-)
> regards,
> dan carpenter
> 
> .
>