[PATCH v2 0/4] block: add IOC_PR_READ_KEYS and IOC_PR_READ_RESERVATION ioctls

[PATCH v2 0/4] block: add IOC_PR_READ_KEYS and IOC_PR_READ_RESERVATION ioctls

[Stefan Hajnoczi]

v2:
- Fix num_keys validation in patches 1-3 [Hannes]
- Declare local variables at beginning of scope [Hannes]

This series exposes struct pr_ops pr_read_keys() and pr_read_reservations() to
userspace as ioctls, making it possible to list registered reservation keys and
report the current reservation on a block device.

The new ioctls are needed by applications or cluster managers that rely on
inspecting the PR state. This is something that has been possible with SCSI-
and NVME-specific commands but not with the PR ioctls. I hope to move QEMU from
SG_IO to PR ioctls so that NVMe host block devices can be supported alongside
SCSI devices without protocol-specific commands.

These ioctls will also make troubleshooting possible with the blkpr(8)
util-linux tool, for which I have prepared a separate patch series.

Stefan Hajnoczi (4):
  scsi: sd: reject invalid pr_read_keys() num_keys values
  nvme: reject invalid pr_read_keys() num_keys values
  block: add IOC_PR_READ_KEYS ioctl
  block: add IOC_PR_READ_RESERVATION ioctl

 include/uapi/linux/pr.h | 14 +++++++
 block/ioctl.c           | 87 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/pr.c  |  4 ++
 drivers/scsi/sd.c       | 11 +++++-
 4 files changed, 115 insertions(+), 1 deletion(-)

-- 
2.52.0

[PATCH v2 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

The pr_read_keys() interface has a u32 num_keys parameter. The SCSI
PERSISTENT RESERVE IN command has a maximum READ KEYS service action
size of 65536 bytes. Reject num_keys values that are too large to fit
into the SCSI command.

This will become important when pr_read_keys() is exposed to untrusted
userspace via an <linux/pr.h> ioctl.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/scsi/sd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 0252d3f6bed17..e436ed977cdb4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1974,9 +1974,18 @@ static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info)
 {
 	int result, i, data_offset, num_copy_keys;
 	u32 num_keys = keys_info->num_keys;
-	int data_len = num_keys * 8 + 8;
+	int data_len;
 	u8 *data;
 
+	/*
+	 * Each reservation key takes 8 bytes and there is an 8-byte header
+	 * before the reservation key list. The total size must fit into the
+	 * 16-bit ALLOCATION LENGTH field.
+	 */
+	if (num_keys > (USHRT_MAX / 8) - 1)
+		return -EINVAL;
+
+	data_len = num_keys * 8 + 8;
 	data = kzalloc(data_len, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.52.0

[PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
Reservation Report command has a u32 maximum length. Reject num_keys
values that are too large to fit.

This will become important when pr_read_keys() is exposed to untrusted
userspace via an <linux/pr.h> ioctl.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/nvme/host/pr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index ca6a74607b139..156a2ae1fac2e 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -233,6 +233,10 @@ static int nvme_pr_read_keys(struct block_device *bdev,
 	int ret, i;
 	bool eds;
 
+	/* Check that keys fit into u32 rse_len */
+	if (num_keys > (U32_MAX - sizeof(*rse)) / sizeof(rse->regctl_eds[0]))
+		return -EINVAL;
+
 	/*
 	 * Assume we are using 128-bit host IDs and allocate a buffer large
 	 * enough to get enough keys to fill the return keys buffer.
-- 
2.52.0

[PATCH v2 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

Add a Persistent Reservations ioctl to read the list of currently
registered reservation keys. This calls the pr_ops->read_keys() function
that was previously added in commit c787f1baa503 ("block: Add PR
callouts for read keys and reservation") but was only used by the
in-kernel SCSI target so far.

The IOC_PR_READ_KEYS ioctl is necessary so that userspace applications
that rely on Persistent Reservations ioctls have a way of inspecting the
current state. Cluster managers and validation tests need this
functionality.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/uapi/linux/pr.h |  7 +++++
 block/ioctl.c           | 59 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index d8126415966f3..fcb74eab92c80 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -56,6 +56,12 @@ struct pr_clear {
 	__u32	__pad;
 };
 
+struct pr_read_keys {
+	__u32	generation;
+	__u32	num_keys;
+	__u64	keys_ptr;
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
@@ -64,5 +70,6 @@ struct pr_clear {
 #define IOC_PR_PREEMPT		_IOW('p', 203, struct pr_preempt)
 #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
 #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
+#define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
 
 #endif /* _UAPI_PR_H */
diff --git a/block/ioctl.c b/block/ioctl.c
index d7489a56b33c3..63b942392b234 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/capability.h>
+#include <linux/cleanup.h>
 #include <linux/compat.h>
 #include <linux/blkdev.h>
 #include <linux/export.h>
@@ -423,6 +424,62 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
 	return ops->pr_clear(bdev, c.key);
 }
 
+static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
+		struct pr_read_keys __user *arg)
+{
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_keys *keys_info __free(kfree) = NULL;
+	struct pr_read_keys inout;
+	u64 __user *keys_ptr;
+	size_t keys_info_len;
+	size_t keys_copy_len;
+	u32 num_copy_keys;
+	int ret;
+
+	if (!blkdev_pr_allowed(bdev, mode))
+		return -EPERM;
+	if (!ops || !ops->pr_read_keys)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&inout, arg, sizeof(inout)))
+		return -EFAULT;
+
+	/*
+	 * 64-bit hosts could handle more keys than 32-bit hosts, but this
+	 * limit is more than enough in practice.
+	 */
+	if (inout.num_keys > (U32_MAX - sizeof(*keys_info)) /
+	                     sizeof(keys_info->keys[0]))
+		return -EINVAL;
+
+	keys_info_len = struct_size(keys_info, keys, inout.num_keys);
+	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
+	if (!keys_info)
+		return -ENOMEM;
+
+	keys_info->num_keys = inout.num_keys;
+
+	ret = ops->pr_read_keys(bdev, keys_info);
+	if (ret)
+		return ret;
+
+	/* Copy out individual keys */
+	keys_ptr = u64_to_user_ptr(inout.keys_ptr);
+	num_copy_keys = min(inout.num_keys, keys_info->num_keys);
+	keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
+
+	if (copy_to_user(keys_ptr, keys_info->keys, keys_copy_len))
+		return -EFAULT;
+
+	/* Copy out the arg struct */
+	inout.generation = keys_info->generation;
+	inout.num_keys = keys_info->num_keys;
+
+	if (copy_to_user(arg, &inout, sizeof(inout)))
+		return -EFAULT;
+	return ret;
+}
+
 static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
 		unsigned long arg)
 {
@@ -644,6 +701,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
 		return blkdev_pr_preempt(bdev, mode, argp, true);
 	case IOC_PR_CLEAR:
 		return blkdev_pr_clear(bdev, mode, argp);
+	case IOC_PR_READ_KEYS:
+		return blkdev_pr_read_keys(bdev, mode, argp);
 	default:
 		return blk_get_meta_cap(bdev, cmd, argp);
 	}
-- 
2.52.0

[PATCH v2 4/4] block: add IOC_PR_READ_RESERVATION ioctl

[Stefan Hajnoczi]

Add a Persistent Reservations ioctl to read the current reservation.
This calls the pr_ops->read_reservation() function that was previously
added in commit c787f1baa503 ("block: Add PR callouts for read keys and
reservation") but was only used by the in-kernel SCSI target so far.

The IOC_PR_READ_RESERVATION ioctl is necessary so that userspace
applications that rely on Persistent Reservations ioctls have a way of
inspecting the current state. Cluster managers and validation tests need
this functionality.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 include/uapi/linux/pr.h |  7 +++++++
 block/ioctl.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index fcb74eab92c80..847f3051057af 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -62,6 +62,12 @@ struct pr_read_keys {
 	__u64	keys_ptr;
 };
 
+struct pr_read_reservation {
+	__u64	key;
+	__u32	generation;
+	__u32	type;
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
@@ -71,5 +77,6 @@ struct pr_read_keys {
 #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
 #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
 #define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
+#define IOC_PR_READ_RESERVATION	_IOR('p', 207, struct pr_read_reservation)
 
 #endif /* _UAPI_PR_H */
diff --git a/block/ioctl.c b/block/ioctl.c
index 63b942392b234..a51628236fc7f 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -480,6 +480,32 @@ static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
 	return ret;
 }
 
+static int blkdev_pr_read_reservation(struct block_device *bdev,
+		blk_mode_t mode, struct pr_read_reservation __user *arg)
+{
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_held_reservation rsv = {};
+	struct pr_read_reservation out = {};
+	int ret;
+
+	if (!blkdev_pr_allowed(bdev, mode))
+		return -EPERM;
+	if (!ops || !ops->pr_read_reservation)
+		return -EOPNOTSUPP;
+
+	ret = ops->pr_read_reservation(bdev, &rsv);
+	if (ret)
+		return ret;
+
+	out.key = rsv.key;
+	out.generation = rsv.generation;
+	out.type = rsv.type;
+
+	if (copy_to_user(arg, &out, sizeof(out)))
+		return -EFAULT;
+	return 0;
+}
+
 static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
 		unsigned long arg)
 {
@@ -703,6 +729,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
 		return blkdev_pr_clear(bdev, mode, argp);
 	case IOC_PR_READ_KEYS:
 		return blkdev_pr_read_keys(bdev, mode, argp);
+	case IOC_PR_READ_RESERVATION:
+		return blkdev_pr_read_reservation(bdev, mode, argp);
 	default:
 		return blk_get_meta_cap(bdev, cmd, argp);
 	}
-- 
2.52.0

Re: [PATCH v2 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Hannes Reinecke]

On 11/27/25 16:54, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The SCSI
> PERSISTENT RESERVE IN command has a maximum READ KEYS service action
> size of 65536 bytes. Reject num_keys values that are too large to fit
> into the SCSI command.
> 
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   drivers/scsi/sd.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Hannes Reinecke]

On 11/27/25 16:54, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> Reservation Report command has a u32 maximum length. Reject num_keys
> values that are too large to fit.
> 
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   drivers/nvme/host/pr.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 3/4] block: add IOC_PR_READ_KEYS ioctl

[Hannes Reinecke]

On 11/27/25 16:54, Stefan Hajnoczi wrote:
> Add a Persistent Reservations ioctl to read the list of currently
> registered reservation keys. This calls the pr_ops->read_keys() function
> that was previously added in commit c787f1baa503 ("block: Add PR
> callouts for read keys and reservation") but was only used by the
> in-kernel SCSI target so far.
> 
> The IOC_PR_READ_KEYS ioctl is necessary so that userspace applications
> that rely on Persistent Reservations ioctls have a way of inspecting the
> current state. Cluster managers and validation tests need this
> functionality.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/uapi/linux/pr.h |  7 +++++
>   block/ioctl.c           | 59 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 66 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 0/4] block: add IOC_PR_READ_KEYS and IOC_PR_READ_RESERVATION ioctls

[Martin K. Petersen]

Stefan,

> This series exposes struct pr_ops pr_read_keys() and
> pr_read_reservations() to userspace as ioctls, making it possible to
> list registered reservation keys and report the current reservation on
> a block device.

Looks OK.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen

Re: [PATCH v2 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Christoph Hellwig]

On Thu, Nov 27, 2025 at 10:54:21AM -0500, Stefan Hajnoczi wrote:
> +	/*
> +	 * Each reservation key takes 8 bytes and there is an 8-byte header
> +	 * before the reservation key list. The total size must fit into the
> +	 * 16-bit ALLOCATION LENGTH field.
> +	 */
> +	if (num_keys > (USHRT_MAX / 8) - 1)
> +		return -EINVAL;
> +
> +	data_len = num_keys * 8 + 8;

Having the same arithmerics express here in two different ways is a bit
odd.

I'd expected this to be something like:

	if (check_mul_overflow(num_keys, 8, &data_len) || data_len > USHRT_MAX)
		return -EINVAL;

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Christoph Hellwig]

On Thu, Nov 27, 2025 at 10:54:22AM -0500, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> Reservation Report command has a u32 maximum length. Reject num_keys
> values that are too large to fit.
> 
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  drivers/nvme/host/pr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index ca6a74607b139..156a2ae1fac2e 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -233,6 +233,10 @@ static int nvme_pr_read_keys(struct block_device *bdev,
>  	int ret, i;
>  	bool eds;
>  
> +	/* Check that keys fit into u32 rse_len */
> +	if (num_keys > (U32_MAX - sizeof(*rse)) / sizeof(rse->regctl_eds[0]))
> +		return -EINVAL;
> +

We use struct_size to calculate the size below, which saturates on
overflow.  So just checking the rse_len variable returned by the that
would be nicer.  Bonus points for using sizeof_field() instead of
hardcoding U32_MAX.

Re: [PATCH v2 3/4] block: add IOC_PR_READ_KEYS ioctl

[Christoph Hellwig]

On Thu, Nov 27, 2025 at 10:54:23AM -0500, Stefan Hajnoczi wrote:
> +static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
> +		struct pr_read_keys __user *arg)
> +{
> +	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
> +	struct pr_keys *keys_info __free(kfree) = NULL;

Please avoid the use of the __free mess and write readable and maintainable
code instead.

> +	struct pr_read_keys inout;

Inout is not a very good variable name, as it doesn't really have much
of meaning.  

> +	if (copy_from_user(&inout, arg, sizeof(inout)))
> +		return -EFAULT;
> +
> +	/*
> +	 * 64-bit hosts could handle more keys than 32-bit hosts, but this
> +	 * limit is more than enough in practice.
> +	 */
> +	if (inout.num_keys > (U32_MAX - sizeof(*keys_info)) /
> +	                     sizeof(keys_info->keys[0]))
> +		return -EINVAL;
> +
> +	keys_info_len = struct_size(keys_info, keys, inout.num_keys);

Do the size check on the calculate len here?

> +		return ret;
> +
> +	/* Copy out individual keys */
> +	keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> +	num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> +	keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);

num_copy_keys is only used once, so maybe drop it?

Re: [PATCH v2 4/4] block: add IOC_PR_READ_RESERVATION ioctl

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Chaitanya Kulkarni]

On 11/27/25 07:54, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> Reservation Report command has a u32 maximum length. Reject num_keys
> values that are too large to fit.
>
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
>
> Signed-off-by: Stefan Hajnoczi<stefanha@redhat.com>
> ---
>   drivers/nvme/host/pr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index ca6a74607b139..156a2ae1fac2e 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -233,6 +233,10 @@ static int nvme_pr_read_keys(struct block_device *bdev,
>   	int ret, i;
>   	bool eds;
>   
> +	/* Check that keys fit into u32 rse_len */
> +	if (num_keys > (U32_MAX - sizeof(*rse)) / sizeof(rse->regctl_eds[0]))
> +		return -EINVAL;

de-referencing res in res->regctl_eds[0] is safe in this patch ?

if so please ignore this comment ...

-ck

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Christoph Hellwig]

On Mon, Dec 01, 2025 at 07:11:31AM +0000, Chaitanya Kulkarni wrote:
> On 11/27/25 07:54, Stefan Hajnoczi wrote:
> > The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> > Reservation Report command has a u32 maximum length. Reject num_keys
> > values that are too large to fit.
> >
> > This will become important when pr_read_keys() is exposed to untrusted
> > userspace via an <linux/pr.h> ioctl.
> >
> > Signed-off-by: Stefan Hajnoczi<stefanha@redhat.com>
> > ---
> >   drivers/nvme/host/pr.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> > index ca6a74607b139..156a2ae1fac2e 100644
> > --- a/drivers/nvme/host/pr.c
> > +++ b/drivers/nvme/host/pr.c
> > @@ -233,6 +233,10 @@ static int nvme_pr_read_keys(struct block_device *bdev,
> >   	int ret, i;
> >   	bool eds;
> >   
> > +	/* Check that keys fit into u32 rse_len */
> > +	if (num_keys > (U32_MAX - sizeof(*rse)) / sizeof(rse->regctl_eds[0]))
> > +		return -EINVAL;
> 
> de-referencing res in res->regctl_eds[0] is safe in this patch ?

sizeof does not dereference pointers in the expression.

Re: [PATCH v2 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Mon, Dec 01, 2025 at 07:34:13AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 27, 2025 at 10:54:21AM -0500, Stefan Hajnoczi wrote:
> > +	/*
> > +	 * Each reservation key takes 8 bytes and there is an 8-byte header
> > +	 * before the reservation key list. The total size must fit into the
> > +	 * 16-bit ALLOCATION LENGTH field.
> > +	 */
> > +	if (num_keys > (USHRT_MAX / 8) - 1)
> > +		return -EINVAL;
> > +
> > +	data_len = num_keys * 8 + 8;
> 
> Having the same arithmerics express here in two different ways is a bit
> odd.
> 
> I'd expected this to be something like:
> 
> 	if (check_mul_overflow(num_keys, 8, &data_len) || data_len > USHRT_MAX)
> 		return -EINVAL;

Thanks, will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Mon, Dec 01, 2025 at 07:36:49AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 27, 2025 at 10:54:22AM -0500, Stefan Hajnoczi wrote:
> > The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> > Reservation Report command has a u32 maximum length. Reject num_keys
> > values that are too large to fit.
> > 
> > This will become important when pr_read_keys() is exposed to untrusted
> > userspace via an <linux/pr.h> ioctl.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  drivers/nvme/host/pr.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> > index ca6a74607b139..156a2ae1fac2e 100644
> > --- a/drivers/nvme/host/pr.c
> > +++ b/drivers/nvme/host/pr.c
> > @@ -233,6 +233,10 @@ static int nvme_pr_read_keys(struct block_device *bdev,
> >  	int ret, i;
> >  	bool eds;
> >  
> > +	/* Check that keys fit into u32 rse_len */
> > +	if (num_keys > (U32_MAX - sizeof(*rse)) / sizeof(rse->regctl_eds[0]))
> > +		return -EINVAL;
> > +
> 
> We use struct_size to calculate the size below, which saturates on
> overflow.  So just checking the rse_len variable returned by the that
> would be nicer.  Bonus points for using sizeof_field() instead of
> hardcoding U32_MAX.

Will fix. I don't see how to use sizeof_field() here, but taking
advantage of struct_size() already improves things a lot:

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index ca6a74607b139..ad2ecc2f49a97 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -228,7 +228,8 @@ static int nvme_pr_resv_report(struct block_device *bdev, void *data,
 static int nvme_pr_read_keys(struct block_device *bdev,
                struct pr_keys *keys_info)
 {
-       u32 rse_len, num_keys = keys_info->num_keys;
+       size_t rse_len;
+       u32 num_keys = keys_info->num_keys;
        struct nvme_reservation_status_ext *rse;
        int ret, i;
        bool eds;
@@ -238,6 +239,9 @@ static int nvme_pr_read_keys(struct block_device *bdev,
         * enough to get enough keys to fill the return keys buffer.
         */
        rse_len = struct_size(rse, regctl_eds, num_keys);
+       if (rse_len > U32_MAX)
+               return -EINVAL;
+
        rse = kzalloc(rse_len, GFP_KERNEL);
        if (!rse)
                return -ENOMEM;

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Mon, Dec 01, 2025 at 07:34:13AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 27, 2025 at 10:54:21AM -0500, Stefan Hajnoczi wrote:
> > +	/*
> > +	 * Each reservation key takes 8 bytes and there is an 8-byte header
> > +	 * before the reservation key list. The total size must fit into the
> > +	 * 16-bit ALLOCATION LENGTH field.
> > +	 */
> > +	if (num_keys > (USHRT_MAX / 8) - 1)
> > +		return -EINVAL;
> > +
> > +	data_len = num_keys * 8 + 8;
> 
> Having the same arithmerics express here in two different ways is a bit
> odd.
> 
> I'd expected this to be something like:
> 
> 	if (check_mul_overflow(num_keys, 8, &data_len) || data_len > USHRT_MAX)
> 		return -EINVAL;

Will fix, thanks.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Mon, Dec 01, 2025 at 07:40:16AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 27, 2025 at 10:54:23AM -0500, Stefan Hajnoczi wrote:
> > +static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
> > +		struct pr_read_keys __user *arg)
> > +{
> > +	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
> > +	struct pr_keys *keys_info __free(kfree) = NULL;
> 
> Please avoid the use of the __free mess and write readable and maintainable
> code instead.

Okay.

> > +	struct pr_read_keys inout;
> 
> Inout is not a very good variable name, as it doesn't really have much
> of meaning.  

It's the ioctl argument. I will change it to read_keys in the next
revision. I'm not sure if that's any better, but it reminds us which
struct this is.

> > +	if (copy_from_user(&inout, arg, sizeof(inout)))
> > +		return -EFAULT;
> > +
> > +	/*
> > +	 * 64-bit hosts could handle more keys than 32-bit hosts, but this
> > +	 * limit is more than enough in practice.
> > +	 */
> > +	if (inout.num_keys > (U32_MAX - sizeof(*keys_info)) /
> > +	                     sizeof(keys_info->keys[0]))
> > +		return -EINVAL;
> > +
> > +	keys_info_len = struct_size(keys_info, keys, inout.num_keys);
> 
> Do the size check on the calculate len here?

Yes, that's better. Checking SIZE_MAX also gets rid of the 32-bit vs
64-bit host comment.

> > +		return ret;
> > +
> > +	/* Copy out individual keys */
> > +	keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> > +	num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> > +	keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
> 
> num_copy_keys is only used once, so maybe drop it?

Will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v2 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Christoph Hellwig]

On Mon, Dec 01, 2025 at 11:22:55AM -0500, Stefan Hajnoczi wrote:
> > We use struct_size to calculate the size below, which saturates on
> > overflow.  So just checking the rse_len variable returned by the that
> > would be nicer.  Bonus points for using sizeof_field() instead of
> > hardcoding U32_MAX.
> 
> Will fix. I don't see how to use sizeof_field() here, but taking
> advantage of struct_size() already improves things a lot:

I thought we'd stuff the len in some field, but we actually convert
it to the ndw in the command, so yes it doesn't make sense here.
Sorry for the misleading direction.