linux-kernel - Re: [PATCH V2] brd: check parameter validation before register

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: <20200114091456.GA22268@ming.t460p>
Date:   Tue, 14 Jan 2020 17:16:57 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Zhiqiang Liu <liuzhiqiang26@...wei.com>
Cc:     axboe@...nel.dk, linux-block@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mingfangsen <mingfangsen@...wei.com>, Guiyao <guiyao@...wei.com>,
        zhangsaisai <zhangsaisai@...wei.com>,
        "wubo (T)" <wubo40@...wei.com>
Subject: Re: [PATCH V2] brd: check parameter validation before
 register_blkdev func

On Mon, Jan 13, 2020 at 09:43:23PM +0800, Zhiqiang Liu wrote:
> In brd_init func, rd_nr num of brd_device are firstly allocated
> and add in brd_devices, then brd_devices are traversed to add each
> brd_device by calling add_disk func. When allocating brd_device,
> the disk->first_minor is set to i * max_part, if rd_nr * max_part
> is larger than MINORMASK, two different brd_device may have the same
> devt, then only one of them can be successfully added.

It is just because disk->first_minor is >= 0x100000, then same dev_t
can be allocated in blk_alloc_devt().

	MKDEV(disk->major, disk->first_minor + part->partno)

But block layer does support extended dynamic devt allocation, and brd
sets flag of GENHD_FL_EXT_DEVT too.

So I think the correct fix is to fallback to extended dynamic allocation
when running out of consecutive minor space.

How about the following approach?

And of course, ext devt allocation may fail too, but that is another
generic un-solved issue: error handling isn't done for adding disk.

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index a8730cc4db10..9aa7ce7c9abf 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -398,7 +398,16 @@ static struct brd_device *brd_alloc(int i)
 	if (!disk)
 		goto out_free_queue;
 	disk->major		= RAMDISK_MAJOR;
-	disk->first_minor	= i * max_part;
+
+	/*
+	 * Clear .minors when running out of consecutive minor space since
+	 * GENHD_FL_EXT_DEVT is set, and we can allocate from extended devt
+	 */
+	if ((i * disk->minors) & ~MINORMASK)
+		disk->minors = 0;
+	else
+		disk->first_minor	= i * disk->minors;
+
 	disk->fops		= &brd_fops;
 	disk->private_data	= brd;
 	disk->flags		= GENHD_FL_EXT_DEVT;



Thanks, 
Ming