linux-kernel - Re: [PATCH v1] block: ioprio: Fix ioprio_check

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: <160ecdfc-cb58-47fe-b9ce-fd126acc10fe@kernel.org>
Date:   Fri, 24 Nov 2023 14:53:52 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     Wei Gao <wegao@...e.com>, axboe@...nel.dk, hare@...e.de,
        hch@....de, niklas.cassel@....com, martin.petersen@...cle.com,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] block: ioprio: Fix ioprio_check_cap() validation logic

On 11/24/23 12:05, Wei Gao wrote:
> The current logic "if (level >= IOPRIO_NR_LEVELS)" can not be reached since
> level value get from IOPRIO_PRIO_LEVEL ONLY extract lower 3-bits of ioprio.
> (IOPRIO_NR_LEVELS=8)
> 
> So this trigger LTP test case ioprio_set03 failed, the test case expect
> error when set IOPRIO_CLASS_BE prio 8, in current implementation level
> value will be 0 and obviously can not return error.
> 
> Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition")

No. Please see below.

> Signed-off-by: Wei Gao <wegao@...e.com>
> ---
>  block/ioprio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index b5a942519a79..f83029208f2a 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -33,7 +33,7 @@
>  int ioprio_check_cap(int ioprio)
>  {
>  	int class = IOPRIO_PRIO_CLASS(ioprio);
> -	int level = IOPRIO_PRIO_LEVEL(ioprio);
> +	int data = IOPRIO_PRIO_DATA(ioprio);
>  
>  	switch (class) {
>  		case IOPRIO_CLASS_RT:
> @@ -49,13 +49,13 @@ int ioprio_check_cap(int ioprio)
>  			fallthrough;
>  			/* rt has prio field too */
>  		case IOPRIO_CLASS_BE:
> -			if (level >= IOPRIO_NR_LEVELS)
> +			if (data >= IOPRIO_NR_LEVELS || data < 0)

This is incorrect: data is the combination of level AND hints, so that value can
be larger than or equal to 8 with the level still being valid. Hard NACK on this.

The issue with LTP test case has been fixed in LTP and by changing the ioprio.h
header file. See commit 01584c1e2337 ("scsi: block: Improve ioprio value
validity checks") which introduces IOPRIO_BAD_VALUE() macro for that.

And for ltp, the commits are:
6b7f448fe392 ("ioprio: Use IOPRIO_PRIO_NUM to check prio range")
7c84fa710f75 ("ioprio: use ioprio.h kernel header if it exists")

So please update your setup, including your install of kernel user API header files.

>  				return -EINVAL;
>  			break;
>  		case IOPRIO_CLASS_IDLE:
>  			break;
>  		case IOPRIO_CLASS_NONE:
> -			if (level)
> +			if (data)
>  				return -EINVAL;
>  			break;
>  		case IOPRIO_CLASS_INVALID:

-- 
Damien Le Moal
Western Digital Research