Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 2/2] block: fix use after free for bd_holder_dir
To:     Christoph Hellwig <hch@lst.de>, Yu Kuai <yukuai1@huaweicloud.com>
Cc:     axboe@kernel.dk, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20221103025541.1875809-1-yukuai1@huaweicloud.com>
 <20221103025541.1875809-3-yukuai1@huaweicloud.com>
 <20221103081211.GB3346@lst.de>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <d4b7c2c7-7da5-9814-e934-174cb8cd5740@huaweicloud.com>
Date:   Thu, 3 Nov 2022 17:45:25 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20221103081211.GB3346@lst.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

在 2022/11/03 16:12, Christoph Hellwig 写道:
> On Thu, Nov 03, 2022 at 10:55:41AM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently, the caller of bd_link_disk_holer() get 'bdev' by
>> blkdev_get_by_dev(), which will look up 'bdev' by inode number 'dev'.
>> Howerver, it's possible that del_gendisk() can be called currently, and
>> 'bd_holder_dir' can be freed before bd_link_disk_holer() access it, thus
>> use after free is triggered.
>>
>> t1:				t2:
>> bdev = blkdev_get_by_dev
>> 				del_gendisk
>> 				 kobject_put(bd_holder_dir)
>> 				  kobject_free()
>> bd_link_disk_holder
>>
>> Fix the problem by checking disk is still live and grabbing a reference
>> to 'bd_holder_dir' first in bd_link_disk_holder().
> 
> Looks good with some minor stilistic nipicks:
> 
>> +	if (!disk_live(bdev->bd_disk)) {
>> +		mutex_unlock(&bdev->bd_disk->open_mutex);
>> +		return -ENODEV;
>> +	}
> 
> This can use a goto out_unlock;

This lock is different from current 'out_unlock', add a new lable will
make the code more complex, I think.
> 
>>   		holder->refcnt++;
>> +		kobject_put(bdev->bd_holder_dir);
>>   		goto out_unlock;
>>   	}
>>   
>>   	holder = kzalloc(sizeof(*holder), GFP_KERNEL);
>>   	if (!holder) {
>>   		ret = -ENOMEM;
>> +		kobject_put(bdev->bd_holder_dir);
>>   		goto out_unlock;
>>   	}
>>   
>> @@ -101,16 +114,12 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
>>   		ret = __link_disk_holder(bdev, disk);
>>   		if (ret) {
>>   			kfree(holder);
>> +			kobject_put(bdev->bd_holder_dir);
> 
> And I think a goto out_put_holder and out_free_holder would clean this up
> nicely.

Yes, you're right.
> 
>>   	list_add(&holder->list, &disk->slave_bdevs);
>> -	/*
>> -	 * del_gendisk drops the initial reference to bd_holder_dir, so we need
>> -	 * to keep our own here to allow for cleanup past that point.
>> -	 */
>> -	kobject_get(bdev->bd_holder_dir);
> 
> .. with this then jumping straight to out_unlock.

Ok, I'll do that in next version.
> 
> 
> We should repost a series with my first 7 patches and your two.  I can do
> that, but it might take some time as I just got through (minor) knee
> surgery and am still at the hospital, so if you have spare cycles feel
> free to do it.

I'm glad to do that, and have a good rest ????

Thanks,
Kuai
> 
> .
>