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.
> + 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?
BTW, I think exposing whether ub's state is privileged/unprivileged
to users(./ublk list) is a good idea.
Regards,
Zhang
