Re: [PATCH] block: Fix validation of ioprio level

[Damien Le Moal <dlemoal@kernel.org>]

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@broadcom.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