linux-kernel - Re: [Regression] Cannot overwrite VID header offset any more [Was: [PATCH] ubi: ensure that VID header offset + VID header size <= alloc, size]

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] [day] [month] [year] [list]

Date:   Mon, 17 Apr 2023 18:07:37 +0200 (CEST)
From:   Richard Weinberger <richard@....at>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     George Kennedy <george.kennedy@...cle.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        eorge kennedy <eorge.kennedy@...cle.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        syzkaller <syzkaller@...glegroups.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        harshit m mogalapalli <harshit.m.mogalapalli@...cle.com>,
        kernel <kernel@...gutronix.de>, stable <stable@...r.kernel.org>
Subject: Re: [Regression] Cannot overwrite VID header offset any more [Was:
 [PATCH] ubi: ensure that VID header offset + VID header size <= alloc,
 size]

Uwe,

----- Ursprüngliche Mail -----
> Von: "Uwe Kleine-König" <u.kleine-koenig@...gutronix.de>
> This patch is in mainline as 1b42b1a36fc946f0d7088425b90d491b4257ca3e,
> and backported to various stable releases.
> 
> For me this breaks
> 
>	ubiattach -m 0 -O 2048
> 
> I think the check
> 
>	ubi->vid_hdr_offset + UBI_VID_HDR_SIZE > ubi->vid_hdr_alsize
> 
> is wrong. Without -O passed to ubiattach (and dynamic debug enabled) I
> get:
> 
> [ 5294.936762] UBI DBG gen (pid 9619): sizeof(struct ubi_ainf_peb) 56
> [ 5294.936769] UBI DBG gen (pid 9619): sizeof(struct ubi_wl_entry) 32
> [ 5294.936774] UBI DBG gen (pid 9619): min_io_size      2048
> [ 5294.936779] UBI DBG gen (pid 9619): max_write_size   2048
> [ 5294.936783] UBI DBG gen (pid 9619): hdrs_min_io_size 512
> [ 5294.936787] UBI DBG gen (pid 9619): ec_hdr_alsize    512
> [ 5294.936791] UBI DBG gen (pid 9619): vid_hdr_alsize   512
> [ 5294.936796] UBI DBG gen (pid 9619): vid_hdr_offset   512
> [ 5294.936800] UBI DBG gen (pid 9619): vid_hdr_aloffset 512
> [ 5294.936804] UBI DBG gen (pid 9619): vid_hdr_shift    0
> [ 5294.936808] UBI DBG gen (pid 9619): leb_start        2048
> [ 5294.936812] UBI DBG gen (pid 9619): max_erroneous    409
> 
> So the check would only pass for vid_hdr_offset <= 512 -
> UBI_VID_HDR_SIZE; note that even specifying the default value 512 (i.e.
> 
>	ubiattach -m 0 -O 512
> 
> ) fails the check.
> 
> A less strong check would be:
> 
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 0904eb40c95f..69c28a862430 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -666,8 +666,8 @@ static int io_init(struct ubi_device *ubi, int
> max_beb_per1024)
> 	ubi->ec_hdr_alsize = ALIGN(UBI_EC_HDR_SIZE, ubi->hdrs_min_io_size);
> 	ubi->vid_hdr_alsize = ALIGN(UBI_VID_HDR_SIZE, ubi->hdrs_min_io_size);
> 
> -	if (ubi->vid_hdr_offset && ((ubi->vid_hdr_offset + UBI_VID_HDR_SIZE) >
> -	    ubi->vid_hdr_alsize)) {
> +	if (ubi->vid_hdr_offset &&
> +	    ubi->vid_hdr_offset + UBI_VID_HDR_SIZE > ubi->peb_size) {
> 		ubi_err(ubi, "VID header offset %d too large.", ubi->vid_hdr_offset);
> 		return -EINVAL;
> 	}
> 
> But I'm unsure if this would be too lax?!

As written on IRC, 1e020e1b96af ("ubi: Fix failure attaching when vid_hdr offset equals to (sub)page size") is supposed to fix that
and on it's way into stable.

Thanks,
//richard