Re: [Xen-devel] [PATCH] common/gnttab: Explicitly default to gnttab v1 during domain creation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 09, 2018 at 11:31:58AM +0100, Andrew Cooper wrote:
> For reasons which appear to be exclusively down to poor review of the grant
> table v2 code, a grant table's version field was wasn't initialised during
> creation.
> 
> A number of problems (including XSAs) have occurred in the past trying trying
> to use a grant table which hasn't been properly set up, and various areas of
> the code cope with v0 by defaulting to v1.
> 
> In particular, the toolstack using GNTTABOP_setup_table to be able to fill in
> the store/console grants has a side effect of switching to v1.
> 
> In hindsight however, this "fixup if we see 0" is a very poor, with a
> substantial degree of risk.  Explicitly default to grant table v1 during
> domain create, and let the rest of the code work safely in the knowledge that
> the version is sensibly set.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I would however keep the current gt_version != 0 asserts, and change
the if ( gt_version == 0 ) into ASSERT(gt_version != 0);

> @@ -2991,15 +2970,11 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>  
>      switch ( gt->gt_version )
>      {
> -    case 0:
> -        if ( op.version == 2 )
> -        {
>      case 1:
> -            /* XXX: We could maybe shrink the active grant table here. */
> -            res = gnttab_populate_status_frames(currd, gt, 
> nr_grant_frames(gt));
> -            if ( res < 0)
> -                goto out_unlock;
> -        }
> +        /* XXX: We could maybe shrink the active grant table here. */
> +        res = gnttab_populate_status_frames(currd, gt, nr_grant_frames(gt));
> +        if ( res < 0)
> +            goto out_unlock;
>          break;
>      case 2:
>          for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )

And I would add a default: ASSERT_UNREACHABLE(); here.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel