[PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim

Roger Pau Monne posted 1 patch 3 years, 2 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210108144123.58546-1-roger.pau@citrix.com
There is a newer version of this series
xen/arch/x86/hypercall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Roger Pau Monne 3 years, 2 months ago
A pvshim build doesn't require the grant table functionality built in,
but it does require knowing the number of arguments the hypercall has
so the hypercall parameter clobbering works properly.

Note this hasn't been detected by osstest because the tools pvshim
build is done without debug enabled, so the hypercall parameter
clobbering doesn't happen.

Fixes: d2151152dd2 ('xen: make grant table support configurable')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hypercall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index dd00983005..8d18ef80cc 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
     ARGS(xen_version, 2),
     ARGS(console_io, 3),
     ARGS(physdev_op_compat, 1),
-#ifdef CONFIG_GRANT_TABLE
+#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
     ARGS(grant_table_op, 3),
 #endif
     ARGS(vm_assist, 2),
-- 
2.29.2


Re: [PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Jan Beulich 3 years, 2 months ago
On 08.01.2021 15:41, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
>      ARGS(xen_version, 2),
>      ARGS(console_io, 3),
>      ARGS(physdev_op_compat, 1),
> -#ifdef CONFIG_GRANT_TABLE
> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
>      ARGS(grant_table_op, 3),
>  #endif
>      ARGS(vm_assist, 2),

This is correct when a shim-enabled build runs as shim, but
not when it runs as normal hypervisor. Just like the hypercall
handler gets patched into the hypercall table (in
pv_shim_setup_dom()), the argument count will also want
updating there, I think.

Jan

Re: [PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Roger Pau Monné 3 years, 2 months ago
On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote:
> On 08.01.2021 15:41, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hypercall.c
> > +++ b/xen/arch/x86/hypercall.c
> > @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
> >      ARGS(xen_version, 2),
> >      ARGS(console_io, 3),
> >      ARGS(physdev_op_compat, 1),
> > -#ifdef CONFIG_GRANT_TABLE
> > +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
> >      ARGS(grant_table_op, 3),
> >  #endif
> >      ARGS(vm_assist, 2),
> 
> This is correct when a shim-enabled build runs as shim, but
> not when it runs as normal hypervisor. Just like the hypercall
> handler gets patched into the hypercall table (in
> pv_shim_setup_dom()), the argument count will also want
> updating there, I think.

Having the argument count set when the hypercall handler is NULL is
fine, as Xen won't get into processing hypercall_args_table if the
handler is NULL. While it's true that can be fixed at run time in the
same way that we patch the handler it seems more cumbersome, that's
why I went for this IMO simpler fix.

Thanks, Roger.

Re: [PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Jan Beulich 3 years, 2 months ago
On 08.01.2021 16:11, Roger Pau Monné wrote:
> On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote:
>> On 08.01.2021 15:41, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hypercall.c
>>> +++ b/xen/arch/x86/hypercall.c
>>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
>>>      ARGS(xen_version, 2),
>>>      ARGS(console_io, 3),
>>>      ARGS(physdev_op_compat, 1),
>>> -#ifdef CONFIG_GRANT_TABLE
>>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
>>>      ARGS(grant_table_op, 3),
>>>  #endif
>>>      ARGS(vm_assist, 2),
>>
>> This is correct when a shim-enabled build runs as shim, but
>> not when it runs as normal hypervisor. Just like the hypercall
>> handler gets patched into the hypercall table (in
>> pv_shim_setup_dom()), the argument count will also want
>> updating there, I think.
> 
> Having the argument count set when the hypercall handler is NULL is
> fine, as Xen won't get into processing hypercall_args_table if the
> handler is NULL.

Oh, good point.

> While it's true that can be fixed at run time in the
> same way that we patch the handler it seems more cumbersome, that's
> why I went for this IMO simpler fix.

Agreed:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Roger Pau Monné 3 years, 2 months ago
On Fri, Jan 08, 2021 at 04:24:01PM +0100, Jan Beulich wrote:
> On 08.01.2021 16:11, Roger Pau Monné wrote:
> > On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote:
> >> On 08.01.2021 15:41, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/hypercall.c
> >>> +++ b/xen/arch/x86/hypercall.c
> >>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
> >>>      ARGS(xen_version, 2),
> >>>      ARGS(console_io, 3),
> >>>      ARGS(physdev_op_compat, 1),
> >>> -#ifdef CONFIG_GRANT_TABLE
> >>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
> >>>      ARGS(grant_table_op, 3),
> >>>  #endif
> >>>      ARGS(vm_assist, 2),
> >>
> >> This is correct when a shim-enabled build runs as shim, but
> >> not when it runs as normal hypervisor. Just like the hypercall
> >> handler gets patched into the hypercall table (in
> >> pv_shim_setup_dom()), the argument count will also want
> >> updating there, I think.
> > 
> > Having the argument count set when the hypercall handler is NULL is
> > fine, as Xen won't get into processing hypercall_args_table if the
> > handler is NULL.
> 
> Oh, good point.
> 
> > While it's true that can be fixed at run time in the
> > same way that we patch the handler it seems more cumbersome, that's
> > why I went for this IMO simpler fix.
> 
> Agreed:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

In fact I'm not sure it's helpful to have any build time conditionals
in hypercall_args_table, but let's leave that for another fix.

Roger.

Re: [PATCH] x86/hypercall: fix gnttab hypercall args conditional build on pvshim
Posted by Jan Beulich 3 years, 2 months ago
On 08.01.2021 16:24, Jan Beulich wrote:
> On 08.01.2021 16:11, Roger Pau Monné wrote:
>> On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote:
>>> On 08.01.2021 15:41, Roger Pau Monne wrote:
>>>> --- a/xen/arch/x86/hypercall.c
>>>> +++ b/xen/arch/x86/hypercall.c
>>>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
>>>>      ARGS(xen_version, 2),
>>>>      ARGS(console_io, 3),
>>>>      ARGS(physdev_op_compat, 1),
>>>> -#ifdef CONFIG_GRANT_TABLE
>>>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
>>>>      ARGS(grant_table_op, 3),
>>>>  #endif
>>>>      ARGS(vm_assist, 2),
>>>
>>> This is correct when a shim-enabled build runs as shim, but
>>> not when it runs as normal hypervisor. Just like the hypercall
>>> handler gets patched into the hypercall table (in
>>> pv_shim_setup_dom()), the argument count will also want
>>> updating there, I think.
>>
>> Having the argument count set when the hypercall handler is NULL is
>> fine, as Xen won't get into processing hypercall_args_table if the
>> handler is NULL.
> 
> Oh, good point.

Albeit then - why not drop the #ifdef altogether?

Jan