Re: [PATCH v4 2/4] xen/console: introduce console input permission

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 29 May 2025, dmkhn@xxxxxxxxx wrote:
> Add new flag to domain structure for marking permission to intercept
> the physical console input by the domain.
> 
> Update console input switch logic accordingly.
> 
> No functional change intended.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v3:
> - rebased
> ---
>  xen/arch/arm/vpl011.c      |  2 ++
>  xen/arch/x86/pv/shim.c     |  2 ++
>  xen/common/domain.c        |  2 ++
>  xen/drivers/char/console.c | 18 +++++++++++++++++-
>  xen/include/xen/sched.h    |  8 +++++++-
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 66047bf33c..147958eee8 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -737,6 +737,8 @@ int domain_vpl011_init(struct domain *d, struct 
> vpl011_init_info *info)
>      register_mmio_handler(d, &vpl011_mmio_handler,
>                            vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>  
> +    d->console.input_allowed = true;

This should be set only when backend_in_domain = false.


>      return 0;
>  
>  out1:
> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index c506cc0bec..bc2a7dd5fa 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -238,6 +238,8 @@ void __init pv_shim_setup_dom(struct domain *d, 
> l4_pgentry_t *l4start,
>       * guest from depleting the shim memory pool.
>       */
>      d->max_pages = domain_tot_pages(d);
> +
> +    d->console.input_allowed = true;
>  }
>  
>  static void write_start_info(struct domain *d)
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 87e5be35e5..9bc66d80c4 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -835,6 +835,8 @@ struct domain *domain_create(domid_t domid,
>          flags |= CDF_hardware;
>          if ( old_hwdom )
>              old_hwdom->cdf &= ~CDF_hardware;
> +
> +        d->console.input_allowed = true;
>      }
>  
>      /* Holding CDF_* internal flags. */
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 30701ae0b0..8a0bcff78f 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -512,9 +512,21 @@ static unsigned int __read_mostly console_rx = 0;
>  
>  struct domain *console_get_domain(void)
>  {
> +    struct domain *d;
> +
>      if ( console_rx == 0 )
>              return NULL;
> -    return rcu_lock_domain_by_id(console_rx - 1);
> +
> +    d = rcu_lock_domain_by_id(console_rx - 1);
> +    if ( !d )
> +        return NULL;
> +
> +    if ( d->console.input_allowed )
> +        return d;
> +
> +    rcu_unlock_domain(d);
> +
> +    return NULL;

The original idea was to skip over domains that cannot have any input so
I don't think we should get in this situation. We could even have an
assert.


>  }
>  
>  void console_put_domain(struct domain *d)
> @@ -551,6 +563,10 @@ static void console_switch_input(void)
>          if ( d )
>          {
>              rcu_unlock_domain(d);
> +
> +            if ( !d->console.input_allowed )
> +                break;

shouldn't this be continue instead of break?


>              console_rx = next_rx;
>              printk("*** Serial input to DOM%u", domid);
>              break;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c..e91c99a8f3 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -512,7 +512,7 @@ struct domain
>      bool             auto_node_affinity;
>      /* Is this guest fully privileged (aka dom0)? */
>      bool             is_privileged;
> -    /* Can this guest access the Xen console? */
> +    /* XSM: permission to use HYPERCALL_console_io hypercall */
>      bool             is_console;

While I am in favor of this direction and we certainly need a better way
to distinguish domains that can use HYPERCALL_console_io hypercall from
others, could we simplify this and just assume that "is_console" implies
input_allowed and also set is_console = true in all the same places you
are setting input_allowed = true in this patch?

For clarity, I am suggesting:
- do not add input_allowed
- set is_console = true in domain_vpl011_init, pv_shim_setup_dom, etc.

The only side effect is that we would allow domains with vpl011 to also
use console hypercalls but I don't think there is any harm in that?

I don't feel strongly about this, I am just trying to find ways to make
things simple. I apologize if it was already discussed during review of
one of the previous versions.

I am also OK with this approach.


>      /* Is this guest being debugged by dom0? */
>      bool             debugger_attached;
> @@ -651,6 +651,12 @@ struct domain
>      unsigned int num_llc_colors;
>      const unsigned int *llc_colors;
>  #endif
> +
> +    /* Console settings. */
> +    struct {
> +        /* Permission to take ownership of the physical console input. */
> +        bool input_allowed;
> +    } console;
>  } __aligned(PAGE_SIZE);
>  
>  static inline struct page_list_head *page_to_list(
> -- 
> 2.34.1
> 
>