[PATCH] xen/gntalloc: validate grant count before allocation

[PATCH] xen/gntalloc: validate grant count before allocation

[Yousef Alhouseen]

gntalloc_ioctl_alloc() allocates the grant-id array before checking
whether the requested count can fit within the global grant limit.
Counts above the limit cannot succeed, so reject them before the
user-controlled allocation size reaches kcalloc().

The locked limit check also adds a u32 count to signed global counters.
Rewrite it as a subtraction-based range check so the arithmetic cannot
wrap around the limit.

While there, cast the count before advancing the per-file index so the
page-size multiplication is done in 64-bit arithmetic.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/xen/gntalloc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index eadedd1e9..ba6a25a09 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -272,6 +272,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 	int rc = 0;
 	struct ioctl_gntalloc_alloc_gref op;
 	uint32_t *gref_ids;
+	int limit_snapshot;
 
 	pr_debug("%s: priv %p\n", __func__, priv);
 
@@ -280,6 +281,12 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 		goto out;
 	}
 
+	limit_snapshot = READ_ONCE(limit);
+	if (limit_snapshot < 0 || op.count > (uint32_t)limit_snapshot) {
+		rc = -ENOSPC;
+		goto out;
+	}
+
 	gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_KERNEL);
 	if (!gref_ids) {
 		rc = -ENOMEM;
@@ -292,14 +299,16 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 	 * are about to enforce, removing them here is a good idea.
 	 */
 	do_cleanup();
-	if (gref_size + op.count > limit) {
+	limit_snapshot = READ_ONCE(limit);
+	if (limit_snapshot < 0 || gref_size > limit_snapshot ||
+	    op.count > (uint32_t)(limit_snapshot - gref_size)) {
 		mutex_unlock(&gref_mutex);
 		rc = -ENOSPC;
 		goto out_free;
 	}
 	gref_size += op.count;
 	op.index = priv->index;
-	priv->index += op.count * PAGE_SIZE;
+	priv->index += (uint64_t)op.count * PAGE_SIZE;
 	mutex_unlock(&gref_mutex);
 
 	rc = add_grefs(&op, gref_ids, priv);
-- 
2.54.0

Re: [PATCH] xen/gntalloc: validate grant count before allocation

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2707 bytes --]

On 24.06.26 14:47, Yousef Alhouseen wrote:
> gntalloc_ioctl_alloc() allocates the grant-id array before checking
> whether the requested count can fit within the global grant limit.
> Counts above the limit cannot succeed, so reject them before the
> user-controlled allocation size reaches kcalloc().
> 
> The locked limit check also adds a u32 count to signed global counters.
> Rewrite it as a subtraction-based range check so the arithmetic cannot
> wrap around the limit.
> 
> While there, cast the count before advancing the per-file index so the
> page-size multiplication is done in 64-bit arithmetic.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>   drivers/xen/gntalloc.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index eadedd1e9..ba6a25a09 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -272,6 +272,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
>   	int rc = 0;
>   	struct ioctl_gntalloc_alloc_gref op;
>   	uint32_t *gref_ids;
> +	int limit_snapshot;

This needs to be an unsigned int, same applies to the already existing
"limit" and "gref_ids".

Could you please create a small series with patch 1 changing "limit" and
"gref_ids" to unsigned int and let this patch be patch 2?

The rest of this patch (with the "< 0" tests and some type casts removed)
is looking fine to me.


Juergen

>   
>   	pr_debug("%s: priv %p\n", __func__, priv);
>   
> @@ -280,6 +281,12 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
>   		goto out;
>   	}
>   
> +	limit_snapshot = READ_ONCE(limit);
> +	if (limit_snapshot < 0 || op.count > (uint32_t)limit_snapshot) {
> +		rc = -ENOSPC;
> +		goto out;
> +	}
> +
>   	gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_KERNEL);
>   	if (!gref_ids) {
>   		rc = -ENOMEM;
> @@ -292,14 +299,16 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
>   	 * are about to enforce, removing them here is a good idea.
>   	 */
>   	do_cleanup();
> -	if (gref_size + op.count > limit) {
> +	limit_snapshot = READ_ONCE(limit);
> +	if (limit_snapshot < 0 || gref_size > limit_snapshot ||
> +	    op.count > (uint32_t)(limit_snapshot - gref_size)) {
>   		mutex_unlock(&gref_mutex);
>   		rc = -ENOSPC;
>   		goto out_free;
>   	}
>   	gref_size += op.count;
>   	op.index = priv->index;
> -	priv->index += op.count * PAGE_SIZE;
> +	priv->index += (uint64_t)op.count * PAGE_SIZE;
>   	mutex_unlock(&gref_mutex);
>   
>   	rc = add_grefs(&op, gref_ids, priv);


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 0/2] xen/gntalloc: validate grant count before allocation

[Yousef Alhouseen]

The allocation ioctl currently allocates a user-sized grant-id array
before checking the global grant limit.  It also adds the requested count
when enforcing that limit, which makes the check harder to reason about
in the presence of mixed signed and unsigned types.

Make the grant counters unsigned first, then reject impossible requests
before allocation and use subtraction for the locked limit check.

Changes in v2:
- Split the unsigned type changes into a prerequisite patch.
- Remove the signed checks and unnecessary casts.

Yousef Alhouseen (2):
  xen/gntalloc: make grant counters unsigned
  xen/gntalloc: validate grant count before allocation

 drivers/xen/gntalloc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.54.0

[PATCH v2 1/2] xen/gntalloc: make grant counters unsigned

[Yousef Alhouseen]

The module limit and current allocation count cannot validly be
negative. Give both variables unsigned types so their representation
matches the u32 grant count supplied through the ioctl and negative
module parameter values are rejected by parameter parsing.

This also prepares the limit check for overflow-safe unsigned
arithmetic.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/xen/gntalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index eadedd1e9..9279f1521 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -70,14 +70,14 @@
 #include <xen/gntalloc.h>
 #include <xen/events.h>
 
-static int limit = 1024;
-module_param(limit, int, 0644);
+static unsigned int limit = 1024;
+module_param(limit, uint, 0644);
 MODULE_PARM_DESC(limit, "Maximum number of grants that may be allocated by "
 		"the gntalloc device");
 
 static LIST_HEAD(gref_list);
 static DEFINE_MUTEX(gref_mutex);
-static int gref_size;
+static unsigned int gref_size;
 
 struct notify_info {
 	uint16_t pgoff:12;    /* Bits 0-11: Offset of the byte to clear */
-- 
2.54.0

Re: [PATCH v2 1/2] xen/gntalloc: make grant counters unsigned

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 528 bytes --]

On 27.06.26 00:38, Yousef Alhouseen wrote:
> The module limit and current allocation count cannot validly be
> negative. Give both variables unsigned types so their representation
> matches the u32 grant count supplied through the ioctl and negative
> module parameter values are rejected by parameter parsing.
> 
> This also prepares the limit check for overflow-safe unsigned
> arithmetic.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 2/2] xen/gntalloc: validate grant count before allocation

[Yousef Alhouseen]

gntalloc_ioctl_alloc() allocates the grant-id array before checking
whether the requested count fits within the global grant limit. Counts
above that limit cannot succeed, so reject them before the
user-controlled allocation reaches kcalloc().

Use a subtraction-based check while holding gref_mutex so adding the
requested count cannot wrap. Also cast the count before advancing the
per-file index so the page-size multiplication is performed in 64-bit
arithmetic.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/xen/gntalloc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 9279f1521..3218686be 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -272,6 +272,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 	int rc = 0;
 	struct ioctl_gntalloc_alloc_gref op;
 	uint32_t *gref_ids;
+	unsigned int limit_snapshot;
 
 	pr_debug("%s: priv %p\n", __func__, priv);
 
@@ -280,6 +281,12 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 		goto out;
 	}
 
+	limit_snapshot = READ_ONCE(limit);
+	if (op.count > limit_snapshot) {
+		rc = -ENOSPC;
+		goto out;
+	}
+
 	gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_KERNEL);
 	if (!gref_ids) {
 		rc = -ENOMEM;
@@ -292,14 +299,16 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 	 * are about to enforce, removing them here is a good idea.
 	 */
 	do_cleanup();
-	if (gref_size + op.count > limit) {
+	limit_snapshot = READ_ONCE(limit);
+	if (gref_size > limit_snapshot ||
+	    op.count > limit_snapshot - gref_size) {
 		mutex_unlock(&gref_mutex);
 		rc = -ENOSPC;
 		goto out_free;
 	}
 	gref_size += op.count;
 	op.index = priv->index;
-	priv->index += op.count * PAGE_SIZE;
+	priv->index += (uint64_t)op.count * PAGE_SIZE;
 	mutex_unlock(&gref_mutex);
 
 	rc = add_grefs(&op, gref_ids, priv);
-- 
2.54.0

Re: [PATCH v2 2/2] xen/gntalloc: validate grant count before allocation

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 665 bytes --]

On 27.06.26 00:38, Yousef Alhouseen wrote:
> gntalloc_ioctl_alloc() allocates the grant-id array before checking
> whether the requested count fits within the global grant limit. Counts
> above that limit cannot succeed, so reject them before the
> user-controlled allocation reaches kcalloc().
> 
> Use a subtraction-based check while holding gref_mutex so adding the
> requested count cannot wrap. Also cast the count before advancing the
> per-file index so the page-size multiplication is performed in 64-bit
> arithmetic.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]