diff mbox series

ublk: enforce ublks_max only for unprivileged devices

Message ID 20250228-ublks_max-v1-1-04b7379190c0@purestorage.com (mailing list archive)
State New
Headers show
Series ublk: enforce ublks_max only for unprivileged devices | expand

Checks

Context Check Description
shin/vmtest-linus-master-PR success PR summary
shin/vmtest-linus-master-VM_Test-0 success Logs for build-kernel

Commit Message

Uday Shankar March 1, 2025, 4:31 a.m. UTC 
Commit 403ebc877832 ("ublk_drv: add module parameter of ublks_max for
limiting max allowed ublk dev"), claimed ublks_max was added to prevent
a DoS situation with an untrusted user creating too many ublk devices.
If that's the case, ublks_max should only restrict the number of
unprivileged ublk devices in the system. Enforce the limit only for
unprivileged ublk devices, and rename variables accordingly. Leave the
external-facing parameter name unchanged, since changing it may break
systems which use it (but still update its documentation to reflect its
new meaning).

As a result of this change, in a system where there are only normal
(non-unprivileged) devices, the maximum number of such devices is
increased to 1 << MINORBITS, or 1048576. That ought to be enough for
anyone, right?

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
 drivers/block/ublk_drv.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)


---
base-commit: 590f25e543b895578e10282b8d8fd0701ceea6ca
change-id: 20250228-ublks_max-863ae142b578

Best regards,

Comments

Ming Lei March 1, 2025, 1:46 p.m. UTC | #1 
On Fri, Feb 28, 2025 at 09:31:48PM -0700, Uday Shankar wrote:
> Commit 403ebc877832 ("ublk_drv: add module parameter of ublks_max for
> limiting max allowed ublk dev"), claimed ublks_max was added to prevent
> a DoS situation with an untrusted user creating too many ublk devices.
> If that's the case, ublks_max should only restrict the number of
> unprivileged ublk devices in the system. Enforce the limit only for
> unprivileged ublk devices, and rename variables accordingly. Leave the
> external-facing parameter name unchanged, since changing it may break
> systems which use it (but still update its documentation to reflect its
> new meaning).
> 
> As a result of this change, in a system where there are only normal
> (non-unprivileged) devices, the maximum number of such devices is
> increased to 1 << MINORBITS, or 1048576. That ought to be enough for
> anyone, right?
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming
Jens Axboe March 4, 2025, 3:51 p.m. UTC | #2 
On Fri, 28 Feb 2025 21:31:48 -0700, Uday Shankar wrote:
> Commit 403ebc877832 ("ublk_drv: add module parameter of ublks_max for
> limiting max allowed ublk dev"), claimed ublks_max was added to prevent
> a DoS situation with an untrusted user creating too many ublk devices.
> If that's the case, ublks_max should only restrict the number of
> unprivileged ublk devices in the system. Enforce the limit only for
> unprivileged ublk devices, and rename variables accordingly. Leave the
> external-facing parameter name unchanged, since changing it may break
> systems which use it (but still update its documentation to reflect its
> new meaning).
> 
> [...]

Applied, thanks!

[1/1] ublk: enforce ublks_max only for unprivileged devices
      commit: 80bdfbb3545b6f16680a72c825063d08a6b44c7a

Best regards,
diff mbox series

Patch

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 8d7d6862a80f519d4f5c0a58d1054ead234d118d..d378d2c9b20217bf3e765bcb4914c457d76b7e33 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -495,15 +495,17 @@  static wait_queue_head_t ublk_idr_wq;	/* wait until one idr is freed */
 
 static DEFINE_MUTEX(ublk_ctl_mutex);
 
+
+#define UBLK_MAX_UBLKS UBLK_MINORS
+
 /*
- * Max ublk devices allowed to add
+ * Max unprivileged ublk devices allowed to add
  *
  * It can be extended to one per-user limit in future or even controlled
  * by cgroup.
  */
-#define UBLK_MAX_UBLKS UBLK_MINORS
-static unsigned int ublks_max = 64;
-static unsigned int ublks_added;	/* protected by ublk_ctl_mutex */
+static unsigned int unprivileged_ublks_max = 64;
+static unsigned int unprivileged_ublks_added; /* protected by ublk_ctl_mutex */
 
 static struct miscdevice ublk_misc;
 
@@ -2251,7 +2253,8 @@  static int ublk_add_chdev(struct ublk_device *ub)
 	if (ret)
 		goto fail;
 
-	ublks_added++;
+	if (ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV)
+		unprivileged_ublks_added++;
 	return 0;
  fail:
 	put_device(dev);
@@ -2280,11 +2283,16 @@  static int ublk_add_tag_set(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
+	bool unprivileged;
+
 	ublk_stop_dev(ub);
 	cancel_work_sync(&ub->nosrv_work);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
+	unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 	ublk_put_device(ub);
-	ublks_added--;
+
+	if (unprivileged)
+		unprivileged_ublks_added--;
 }
 
 static struct ublk_device *ublk_get_device_from_id(int idx)
@@ -2549,7 +2557,8 @@  static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 		return ret;
 
 	ret = -EACCES;
-	if (ublks_added >= ublks_max)
+	if ((info.flags & UBLK_F_UNPRIVILEGED_DEV) &&
+	    unprivileged_ublks_added >= unprivileged_ublks_max)
 		goto out_unlock;
 
 	ret = -ENOMEM;
@@ -3183,23 +3192,26 @@  static void __exit ublk_exit(void)
 module_init(ublk_init);
 module_exit(ublk_exit);
 
-static int ublk_set_max_ublks(const char *buf, const struct kernel_param *kp)
+static int ublk_set_max_unprivileged_ublks(const char *buf,
+					   const struct kernel_param *kp)
 {
 	return param_set_uint_minmax(buf, kp, 0, UBLK_MAX_UBLKS);
 }
 
-static int ublk_get_max_ublks(char *buf, const struct kernel_param *kp)
+static int ublk_get_max_unprivileged_ublks(char *buf,
+					   const struct kernel_param *kp)
 {
-	return sysfs_emit(buf, "%u\n", ublks_max);
+	return sysfs_emit(buf, "%u\n", unprivileged_ublks_max);
 }
 
-static const struct kernel_param_ops ublk_max_ublks_ops = {
-	.set = ublk_set_max_ublks,
-	.get = ublk_get_max_ublks,
+static const struct kernel_param_ops ublk_max_unprivileged_ublks_ops = {
+	.set = ublk_set_max_unprivileged_ublks,
+	.get = ublk_get_max_unprivileged_ublks,
 };
 
-module_param_cb(ublks_max, &ublk_max_ublks_ops, &ublks_max, 0644);
-MODULE_PARM_DESC(ublks_max, "max number of ublk devices allowed to add(default: 64)");
+module_param_cb(ublks_max, &ublk_max_unprivileged_ublks_ops,
+		&unprivileged_ublks_max, 0644);
+MODULE_PARM_DESC(ublks_max, "max number of unprivileged ublk devices allowed to add(default: 64)");
 
 MODULE_AUTHOR("Ming Lei <ming.lei@redhat.com>");
 MODULE_DESCRIPTION("Userspace block device");