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

[Juergen Gross]

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@xxxxxxxxx>
> ---
>   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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature