Re: [PATCH 1/2] ublk: Limit dev_id/ub_number values

[Ming Lei <ming.lei@redhat.com>]

On Tue, Oct 03, 2023 at 11:07:37AM -0500, Mike Christie wrote:
> On 10/3/23 10:36 AM, Ming Lei wrote:
> > On Sun, Oct 01, 2023 at 01:54:47PM -0500, Mike Christie wrote:
> >> The dev_id/ub_number is used for the ublk dev's char device's minor
> >> number so it has to fit into MINORMASK. This patch adds checks to prevent
> >> userspace from passing a number that's too large and limits what can be
> >> allocated by the ublk_index_idr for the case where userspace has the
> >> kernel allocate the dev_id/ub_number.
> >>
> >> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> >> ---
> >>  drivers/block/ublk_drv.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> >> index 630ddfe6657b..18e352f8cd6d 100644
> >> --- a/drivers/block/ublk_drv.c
> >> +++ b/drivers/block/ublk_drv.c
> >> @@ -470,6 +470,7 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
> >>   * It can be extended to one per-user limit in future or even controlled
> >>   * by cgroup.
> >>   */
> >> +#define UBLK_MAX_UBLKS (UBLK_MINORS - 1)
> >>  static unsigned int ublks_max = 64;
> >>  static unsigned int ublks_added;	/* protected by ublk_ctl_mutex */
> >>  
> >> @@ -2026,7 +2027,8 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
> >>  		if (err == -ENOSPC)
> >>  			err = -EEXIST;
> >>  	} else {
> >> -		err = idr_alloc(&ublk_index_idr, ub, 0, 0, GFP_NOWAIT);
> >> +		err = idr_alloc(&ublk_index_idr, ub, 0, UBLK_MAX_UBLKS,
> > 
> > 'end' parameter of idr_alloc() is exclusive, so I think UBLK_MAX_UBLKS should
> > be defined as UBLK_MINORS?
> 
> We can use UBLK_MINORS. I just used UBLK_MAX_UBLKS because it was only
> a difference of one device and I thought using UBLK_MAX_UBLKS in the
> all the checks was more consistent.
> 
> But yeah, I can see the opposite where it's more clear to use the
> exact limit and will change it.
> 
> 
> > 
> >> +				GFP_NOWAIT);
> >>  	}
> >>  	spin_unlock(&ublk_idr_lock);
> >>  
> >> @@ -2305,6 +2307,12 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> +	if (header->dev_id != U32_MAX && header->dev_id > UBLK_MAX_UBLKS) {
> > 
> > I guess 'if (header->dev_id >= UBLK_MAX_UBLKS)' should be enough.
> 
> I can't drop the first part of the check because header->dev_id is a
> u32:

Your are right, let's keep the check.

> 
> struct ublksrv_ctrl_cmd {
>         /* sent to which device, must be valid */
>         __u32   dev_id;
> 
> and userspace is passing in:
> 
> dev_id = U32_MAX
> 
> to indicate for the kernel to allocate the dev_id.
> 
> 
> The weirdness is that we convert dev_id to a int later:
> 
> ret = ublk_alloc_dev_number(ub, header->dev_id);
> 
> ....
> 
> static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
> 
> so the header->dev_id gets converted to a signed int and in
> ublk_alloc_dev_number U32_MAX gets turned into -1. There
> we check the idx/dev_id more similar to what you suggested above.

The thing is that '-1' means auto-id-allocation, and the .dev_id field
should have been defined as -1 from beginning, but it can't change now.

thanks,
Ming