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