Re: [PATCH v3 4/6] vpci: restrict unhandled read/write operations for guests

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

On Tue, Mar 14, 2023 at 08:56:30PM +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> A guest would be able to read and write those registers which are not
> emulated and have no respective vPCI handlers, so it will be possible
> for it to access the hardware directly.
> In order to prevent a guest from reads and writes from/to the unhandled
> registers make sure only hardware domain can access the hardware directly
> and restrict guests from doing so.
> 
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> ---
> 
> v3:
>  - No changes
> 
> Older comments from another series:
> 
> Since v6:
> - do not use is_hwdom parameter for vpci_{read|write}_hw and use
>   current->domain internally
> - update commit message
> New in v6
> Moved into another series
> ---
>  xen/drivers/vpci/vpci.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 5232f9605b..199ff55672 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -220,6 +220,10 @@ static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned 
> int reg,
>  {
>      uint32_t data;
>  
> +    /* Guest domains are not allowed to read real hardware. */
> +    if ( !is_hardware_domain(current->domain) )
> +        return ~(uint32_t)0;
> +
>      switch ( size )
>      {
>      case 4:
> @@ -260,9 +264,13 @@ static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned 
> int reg,
>      return data;
>  }
>  
> -static void vpci_write_hw(pci_sbdf_t sbdf, unsigned int reg, unsigned int 
> size,
> -                          uint32_t data)
> +static void vpci_write_hw(pci_sbdf_t sbdf, unsigned int reg,
> +                          unsigned int size, uint32_t data)

Unrelated change? The parameter list doesn't go over 80 characters so
this rearranging is not necessary, and in any case should be done in a
separate commit or at least mentioned in the commit log.

>  {
> +    /* Guest domains are not allowed to write real hardware. */

I would maybe write this as:

"Unprivileged domain are not allowed unhandled accesses to the config
space."

But that's mostly a nit, and would also apply to the comment in
vpci_read_hw().

Thanks, Roger.