Re: [PATCH 2/6] ublk_drv: don't probe partitions if the ubq daemon isn't trusted

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

On Fri, Nov 18, 2022 at 06:03:48PM +0800, Ziyang Zhang wrote:
> On 2022/11/16 14:08, Ming Lei wrote:
> > If any ubq daemon is unprivileged, the ublk char device is allowed
> > for unprivileged user, and we can't trust the current user, so not
> > probe partitions.
> > 
> > Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index fe997848c1ff..a5f3d8330be5 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -149,6 +149,7 @@ struct ublk_device {
> >  
> >  #define UB_STATE_OPEN		0
> >  #define UB_STATE_USED		1
> > +#define UB_STATE_PRIVILEGED	2
> >  	unsigned long		state;
> >  	int			ub_number;
> >  
> > @@ -161,6 +162,7 @@ struct ublk_device {
> >  
> >  	struct completion	completion;
> >  	unsigned int		nr_queues_ready;
> > +	unsigned int		nr_privileged_daemon;
> >  
> >  	/*
> >  	 * Our ubq->daemon may be killed without any notification, so
> > @@ -1184,9 +1186,15 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
> >  		ubq->ubq_daemon = current;
> >  		get_task_struct(ubq->ubq_daemon);
> >  		ub->nr_queues_ready++;
> > +
> > +		if (capable(CAP_SYS_ADMIN))
> > +			ub->nr_privileged_daemon++;
> >  	}
> > -	if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues)
> > +	if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues) {
> > +		if (ub->nr_privileged_daemon == ub->nr_queues_ready)
> 
> Hi, Ming.
> 
> Just like nr_queues_ready, ub->nr_privileged_daemon should be reset
> to zero in ublk_ctrl_start_recovery(). otherwise new ubq_daemons are
> always treated as unprivileged.

Good catch!

> 
> > +			set_bit(UB_STATE_PRIVILEGED, &ub->state);
> >  		complete_all(&ub->completion);
> > +	}
> >  	mutex_unlock(&ub->mutex);
> >  }
> >  
> > @@ -1540,6 +1548,10 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
> >  	if (ret)
> >  		goto out_put_disk;
> >  
> > +	/* don't probe partitions if any one ubq daemon is un-trusted */
> > +	if (!test_bit(UB_STATE_PRIVILEGED, &ub->state))
> > +		set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
> 
> Can we simply check if nr_queues_ready == nr_privileged_daemon here
> instead of adding a new bit UB_STATE_PRIVILEGED?

Good idea!

> 
> BTW, I think exposing whether ub's state is privileged/unprivileged
> to users(./ublk list) is a good idea.

It is actually not a state, but a flag of UBLK_F_UNPRIVILEGED_DEV, which
won't be changed for one device and is shown in 'ublk list'.

For root user, maybe we should clear the flag from UBLK_CMD_ADD_DEV.

Thanks,
Ming