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

[Juergen Gross <jgross@xxxxxxxx>]

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