linux-kernel - Re: [PATCH 02/14] ubi: Fix assert in ubi_wl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180620121116.09996426@bbrezillon>
Date:   Wed, 20 Jun 2018 12:11:16 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Richard Weinberger <richard@....at>
Cc:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/14] ubi: Fix assert in ubi_wl_init

On Wed, 13 Jun 2018 23:23:32 +0200
Richard Weinberger <richard@....at> wrote:

> When multiple PEBs are used for a fastmap, found_pebs
> can be wrong and the assert triggers.
> 
> The current approach is broken in two ways:
> 1. The "continue" in list_for_each_entry() over all fastmap PEBs
>    misses the counter at all.
> 2. When the fastmap changes in size, growing due to a preseeded fastmap
>    or shrinking due to new bad blocks, iterating over the current
>    fastmap will give wrong numbers. We have to exclude the fastmap size
>    at all from the calculation to be able to compare the numbers.
>    At this stage we simply have no longer the information whether the
>    fastmap changed in size.

Should this patch be backported to stable releases? You say the problem
arises when new bad blocks appear, so it's not only a problem you'll
have with the preseeded fastmap changes.

> 
> Signed-off-by: Richard Weinberger <richard@....at>
> ---
>  drivers/mtd/ubi/wl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index f66b3b22f328..6bbb968fe9da 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1695,11 +1695,19 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  			err = erase_aeb(ubi, aeb, sync);
>  			if (err)
>  				goto out_free;
> -		}
>  
> -		found_pebs++;
> +			/*
> +			 * If no fastmap is used, all fastmap PEBs will get be

				remove either "get" or "be" here ^

> +			 * erased and are member of ai->fastmap.
> +			 */
> +			if (!ubi->fm)
> +				found_pebs++;
> +		}
>  	}
>  
> +	if (ubi->fm)
> +		found_pebs += ubi->fm->used_blocks;
> +

Hm, are we sure this is always correct? I mean, what if you had an old
fastmap scheduled for erasure but a power-cut happened before it was
erased. Are you sure we won't have an inconsistent found_pebs number
(found_pebs != ubi->good_peb_count).

I understand that this problem already exists because of

	if (ubi->lookuptbl[aeb->pnum])
		continue;

but I'm not sure your solution fixes that.

>  	dbg_wl("found %i PEBs", found_pebs);
>  
>  	ubi_assert(ubi->good_peb_count == found_pebs);