[PATCH] block: move capacity validation to blkpg_do_ioctl()

[PATCH] block: move capacity validation to blkpg_do_ioctl()

[Li Lingfeng]

From: Li Lingfeng <lilingfeng3@huawei.com>

Commit 6d4e80db4ebe ("block: add capacity validation in
bdev_add_partition()") add check of partition's start and end sectors to
prevent exceeding the size of the disk when adding partitions. However,
there is still no check for resizing partitions now.
Move the check to blkpg_do_ioctl() to cover resizing partitions.

Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
 block/ioctl.c           |  9 ++++++++-
 block/partitions/core.c | 11 -----------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 438f79c564cf..de0cc0d215c6 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -18,7 +18,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
 {
 	struct gendisk *disk = bdev->bd_disk;
 	struct blkpg_partition p;
-	sector_t start, length;
+	sector_t start, length, capacity, end;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -41,6 +41,13 @@ static int blkpg_do_ioctl(struct block_device *bdev,
 
 	start = p.start >> SECTOR_SHIFT;
 	length = p.length >> SECTOR_SHIFT;
+	capacity = get_capacity(disk);
+
+	if (check_add_overflow(start, length, &end))
+		return -EINVAL;
+
+	if (start >= capacity || end > capacity)
+		return -EINVAL;
 
 	switch (op) {
 	case BLKPG_ADD_PARTITION:
diff --git a/block/partitions/core.c b/block/partitions/core.c
index 5f5ed5c75f04..b11e88c82c8c 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -419,21 +419,10 @@ static bool partition_overlaps(struct gendisk *disk, sector_t start,
 int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
 		sector_t length)
 {
-	sector_t capacity = get_capacity(disk), end;
 	struct block_device *part;
 	int ret;
 
 	mutex_lock(&disk->open_mutex);
-	if (check_add_overflow(start, length, &end)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (start >= capacity || end > capacity) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	if (!disk_live(disk)) {
 		ret = -ENXIO;
 		goto out;
-- 
2.39.2

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Damien Le Moal]

On 3/5/24 12:21, Li Lingfeng wrote:
> From: Li Lingfeng <lilingfeng3@huawei.com>
> 
> Commit 6d4e80db4ebe ("block: add capacity validation in
> bdev_add_partition()") add check of partition's start and end sectors to
> prevent exceeding the size of the disk when adding partitions. However,
> there is still no check for resizing partitions now.
> Move the check to blkpg_do_ioctl() to cover resizing partitions.
> 
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
>  block/ioctl.c           |  9 ++++++++-
>  block/partitions/core.c | 11 -----------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 438f79c564cf..de0cc0d215c6 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -18,7 +18,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>  {
>  	struct gendisk *disk = bdev->bd_disk;
>  	struct blkpg_partition p;
> -	sector_t start, length;
> +	sector_t start, length, capacity, end;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
> @@ -41,6 +41,13 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>  
>  	start = p.start >> SECTOR_SHIFT;
>  	length = p.length >> SECTOR_SHIFT;
> +	capacity = get_capacity(disk);
> +
> +	if (check_add_overflow(start, length, &end))
> +		return -EINVAL;
> +
> +	if (start >= capacity || end > capacity)
> +		return -EINVAL;
>  
>  	switch (op) {
>  	case BLKPG_ADD_PARTITION:
> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index 5f5ed5c75f04..b11e88c82c8c 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c
> @@ -419,21 +419,10 @@ static bool partition_overlaps(struct gendisk *disk, sector_t start,
>  int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
>  		sector_t length)
>  {
> -	sector_t capacity = get_capacity(disk), end;
>  	struct block_device *part;
>  	int ret;
>  
>  	mutex_lock(&disk->open_mutex);
> -	if (check_add_overflow(start, length, &end)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (start >= capacity || end > capacity) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -

Why do you remove this ? The check will not be done when *existing* partitions
are added. To do the check when *creating* a partition, make this code a helper
and call that helper function here and from blkpg_do_ioctl() as well.

>  	if (!disk_live(disk)) {
>  		ret = -ENXIO;
>  		goto out;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Li Lingfeng]

在 2024/3/5 12:30, Damien Le Moal 写道:
> On 3/5/24 12:21, Li Lingfeng wrote:
>> From: Li Lingfeng <lilingfeng3@huawei.com>
>>
>> Commit 6d4e80db4ebe ("block: add capacity validation in
>> bdev_add_partition()") add check of partition's start and end sectors to
>> prevent exceeding the size of the disk when adding partitions. However,
>> there is still no check for resizing partitions now.
>> Move the check to blkpg_do_ioctl() to cover resizing partitions.
>>
>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>> ---
>>   block/ioctl.c           |  9 ++++++++-
>>   block/partitions/core.c | 11 -----------
>>   2 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 438f79c564cf..de0cc0d215c6 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -18,7 +18,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>>   {
>>   	struct gendisk *disk = bdev->bd_disk;
>>   	struct blkpg_partition p;
>> -	sector_t start, length;
>> +	sector_t start, length, capacity, end;
>>   
>>   	if (!capable(CAP_SYS_ADMIN))
>>   		return -EACCES;
>> @@ -41,6 +41,13 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>>   
>>   	start = p.start >> SECTOR_SHIFT;
>>   	length = p.length >> SECTOR_SHIFT;
>> +	capacity = get_capacity(disk);
>> +
>> +	if (check_add_overflow(start, length, &end))
>> +		return -EINVAL;
>> +
>> +	if (start >= capacity || end > capacity)
>> +		return -EINVAL;
>>   
>>   	switch (op) {
>>   	case BLKPG_ADD_PARTITION:
>> diff --git a/block/partitions/core.c b/block/partitions/core.c
>> index 5f5ed5c75f04..b11e88c82c8c 100644
>> --- a/block/partitions/core.c
>> +++ b/block/partitions/core.c
>> @@ -419,21 +419,10 @@ static bool partition_overlaps(struct gendisk *disk, sector_t start,
>>   int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
>>   		sector_t length)
>>   {
>> -	sector_t capacity = get_capacity(disk), end;
>>   	struct block_device *part;
>>   	int ret;
>>   
>>   	mutex_lock(&disk->open_mutex);
>> -	if (check_add_overflow(start, length, &end)) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>> -	if (start >= capacity || end > capacity) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
> Why do you remove this ? The check will not be done when *existing* partitions
I'm sorry, I didn't quite understand your point. The function 
bdev_add_partition() is only called within blkpg_do_ioctl(). I simply 
moved the check to a different location, but did not remove it entirely.
> are added. To do the check when *creating* a partition, make this code a helper
> and call that helper function here and from blkpg_do_ioctl() as well.
Do you mean call the helper function in blk_add_partition() and 
blkpg_do_ioctl()?
>
>>   	if (!disk_live(disk)) {
>>   		ret = -ENXIO;
>>   		goto out;

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Damien Le Moal]

On 3/5/24 15:24, Li Lingfeng wrote:
> 
> 在 2024/3/5 12:30, Damien Le Moal 写道:
>> On 3/5/24 12:21, Li Lingfeng wrote:
>>> From: Li Lingfeng <lilingfeng3@huawei.com>
>>>
>>> Commit 6d4e80db4ebe ("block: add capacity validation in
>>> bdev_add_partition()") add check of partition's start and end sectors to
>>> prevent exceeding the size of the disk when adding partitions. However,
>>> there is still no check for resizing partitions now.
>>> Move the check to blkpg_do_ioctl() to cover resizing partitions.
>>>
>>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>>> ---
>>>   block/ioctl.c           |  9 ++++++++-
>>>   block/partitions/core.c | 11 -----------
>>>   2 files changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/ioctl.c b/block/ioctl.c
>>> index 438f79c564cf..de0cc0d215c6 100644
>>> --- a/block/ioctl.c
>>> +++ b/block/ioctl.c
>>> @@ -18,7 +18,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>>>   {
>>>       struct gendisk *disk = bdev->bd_disk;
>>>       struct blkpg_partition p;
>>> -    sector_t start, length;
>>> +    sector_t start, length, capacity, end;
>>>         if (!capable(CAP_SYS_ADMIN))
>>>           return -EACCES;
>>> @@ -41,6 +41,13 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>>>         start = p.start >> SECTOR_SHIFT;
>>>       length = p.length >> SECTOR_SHIFT;
>>> +    capacity = get_capacity(disk);
>>> +
>>> +    if (check_add_overflow(start, length, &end))
>>> +        return -EINVAL;
>>> +
>>> +    if (start >= capacity || end > capacity)
>>> +        return -EINVAL;
>>>         switch (op) {
>>>       case BLKPG_ADD_PARTITION:
>>> diff --git a/block/partitions/core.c b/block/partitions/core.c
>>> index 5f5ed5c75f04..b11e88c82c8c 100644
>>> --- a/block/partitions/core.c
>>> +++ b/block/partitions/core.c
>>> @@ -419,21 +419,10 @@ static bool partition_overlaps(struct gendisk *disk,
>>> sector_t start,
>>>   int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
>>>           sector_t length)
>>>   {
>>> -    sector_t capacity = get_capacity(disk), end;
>>>       struct block_device *part;
>>>       int ret;
>>>         mutex_lock(&disk->open_mutex);
>>> -    if (check_add_overflow(start, length, &end)) {
>>> -        ret = -EINVAL;
>>> -        goto out;
>>> -    }
>>> -
>>> -    if (start >= capacity || end > capacity) {
>>> -        ret = -EINVAL;
>>> -        goto out;
>>> -    }
>>> -
>> Why do you remove this ? The check will not be done when *existing* partitions
> I'm sorry, I didn't quite understand your point. The function
> bdev_add_partition() is only called within blkpg_do_ioctl(). I simply moved the
> check to a different location, but did not remove it entirely.

Oh ! My bad ! I got confused with blk_add_partition() which is the function
adding partitions when a new disk is scanned. So yes, your patch is fine. My
apologies for the noise.

>> are added. To do the check when *creating* a partition, make this code a helper
>> and call that helper function here and from blkpg_do_ioctl() as well.
> Do you mean call the helper function in blk_add_partition() and blkpg_do_ioctl()?
>>
>>>       if (!disk_live(disk)) {
>>>           ret = -ENXIO;
>>>           goto out;
> 
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Damien Le Moal]

On 3/5/24 12:21, Li Lingfeng wrote:
> From: Li Lingfeng <lilingfeng3@huawei.com>
> 
> Commit 6d4e80db4ebe ("block: add capacity validation in
> bdev_add_partition()") add check of partition's start and end sectors to
> prevent exceeding the size of the disk when adding partitions. However,
> there is still no check for resizing partitions now.
> Move the check to blkpg_do_ioctl() to cover resizing partitions.
> 
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

But I wonder if this needs a Fixes: 6d4e80db4ebe ("block: add capacity
validation in bdev_add_partition()") tag and Cc-stable...

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: move capacity validation to blkpg_do_ioctl()

[Jens Axboe]

On Tue, 05 Mar 2024 11:21:32 +0800, Li Lingfeng wrote:
> Commit 6d4e80db4ebe ("block: add capacity validation in
> bdev_add_partition()") add check of partition's start and end sectors to
> prevent exceeding the size of the disk when adding partitions. However,
> there is still no check for resizing partitions now.
> Move the check to blkpg_do_ioctl() to cover resizing partitions.
> 
> 
> [...]

Applied, thanks!

[1/1] block: move capacity validation to blkpg_do_ioctl()
      commit: b9355185d2aeef11f6e365dd98def82212637936

Best regards,
-- 
Jens Axboe