linux-kernel - Re: [PATCH] block: Fix validation of ioprio level

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: <a5609ba3-cc35-41c5-98f1-52063f8a6eec@kernel.org>
Date: Wed, 28 Aug 2024 17:45:36 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Ajay Kaher <ajay.kaher@...adcom.com>, axboe@...nel.dk,
 niklas.cassel@....com, hare@...e.de, martin.petersen@...cle.com
Cc: linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 alexey.makhalov@...adcom.com, vasavi.sirnapalli@...adcom.com,
 vamsi-krishna.brahmajosyula@...adcom.com
Subject: Re: [PATCH] block: Fix validation of ioprio level

On 8/28/24 17:28, Ajay Kaher wrote:
> The commit eca2040972b4 introduced a backward compatibility issue in
> the function ioprio_check_cap.
> 
> Before the change, if ioprio contains a level greater than 0x7, it was
> treated as -EINVAL:
> 
>     data = ioprio & 0x1FFF
>     if data >= 0x7, return -EINVAL 
> 
> Since the change, if ioprio contains a level greater than 0x7 say 0x8
> it is calculated as 0x0:
> 
>     level = ioprio & 0x7
> 
> To maintain backward compatibility the kernel should return -EINVAL in
> the above case as well.
> 
> Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition")
> Signed-off-by: Ajay Kaher <ajay.kaher@...adcom.com>
> ---
>  block/ioprio.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 73301a2..f08e76b 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -30,6 +30,15 @@
>  #include <linux/security.h>
>  #include <linux/pid_namespace.h>
>  
> +static inline int ioprio_check_level(int ioprio, int max_level)
> +{
> +	int data = IOPRIO_PRIO_DATA(ioprio);
> +
> +	if (IOPRIO_BAD_VALUE(data, max_level))
> +		return -EINVAL;

No, this cannot possibly work correctly because the prio level part of the prio
data is only 3 bits, so 0 to 7. The remaining 10 bits of the prio data are used
for priority hints (IOPRIO_HINT_XXX).

Your change will thus return an error for cases where the prio data has a level
AND also a hint (e.g. for command duration limits). This change would break
command duration limits. So NACK.

The userspace header file has the ioprio_value() that a user should use to
construct an ioprio. Bad values are checked in that function and errors will be
returned if an invalid level is passed.

> +	return 0;
> +}
> +
>  int ioprio_check_cap(int ioprio)
>  {
>  	int class = IOPRIO_PRIO_CLASS(ioprio);
> @@ -49,7 +58,7 @@ int ioprio_check_cap(int ioprio)
>  			fallthrough;
>  			/* rt has prio field too */
>  		case IOPRIO_CLASS_BE:
> -			if (level >= IOPRIO_NR_LEVELS)
> +			if (ioprio_check_level(ioprio, IOPRIO_NR_LEVELS))
>  				return -EINVAL;
>  			break;
>  		case IOPRIO_CLASS_IDLE:

-- 
Damien Le Moal
Western Digital Research