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@xxxxxxxxxx>
> >> ---
> >> 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
