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

[Ajay Kaher]

On Wed, Aug 28, 2024 at 2:15 PM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> > ---
> >  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.
>

OK. Thanks for the detailed explanation.

I agree, to use unused bits, functionality (return value in this case)
will be changed. If applications are built using Kernel headers of
v6.1 (doesn't include eca2040972b4) and later only upgrading Kernel to
v6.6, because of the changes in return values applications may have
some sort of regression.

To make the software backward compatible I believe, unused bits should
always be ignored. So that if in future someone uses it, it should not
change the behaviour (return values) of existing software.

- Ajay

> > +     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
>