Re: [PATCH v7 6/7] xen/serial: harden serial_tx_buffer checks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2026 20:16, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Ensure the user-defined value never crosses 2GB boundary and always
> rounded to the next power of 2 to align with console driver conring
> buffer management code.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - new patch
> ---
>  docs/misc/xen-command-line.pandoc |  2 ++
>  xen/drivers/char/serial.c         | 21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 1c711fa98086..2be8772b329a 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2396,6 +2396,8 @@ accidentally leaking secrets by releasing pages without 
> proper sanitization.
>  
>  Set the serial transmit buffer size.
>  
> +The value provided will be rounded down to the nearest power of 2.
> +
>  ### serrors (ARM)
>  > `= diverse | panic`
>  
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -16,7 +16,26 @@
>  #include <asm/processor.h>
>  
>  unsigned int __ro_after_init serial_txbufsz = CONFIG_SERIAL_TX_BUFSIZE;
> -size_param("serial_tx_buffer", serial_txbufsz);

Before making this change, surely you did look at the difference between
size_param() and integer_param()? You should have noticed that you regress
behavior by ...

> +static int __init cf_check parse_serial_txbufsz(const char *s)
> +{
> +    long long val;
> +
> +    if ( parse_signed_integer("serial_tx_buffer", s, NULL, &val) )
> +        return 0;

... using this function. It's additionally unclear why a signed value
would want parsing here: What's a ring of negative size?

> +    if ( val < KB(16) || val > GB(2) )
> +    {
> +        printk(XENLOG_WARNING "'serial_tx_buffer=%lld' value out of range, "
> +               "falling back to default\n", val);
> +        val = CONFIG_SERIAL_TX_BUFSIZE;

In the earlier patch you used the upper bound for too large a value. Why
does it become the default size here in such a case?

> +    }
> +
> +    serial_txbufsz = PAGE_SIZE << get_order_from_bytes(val);
> +
> +    return 0;
> +}
> +custom_param("serial_tx_buffer", parse_serial_txbufsz);

Overall you're duplicating behavior that serial_async_transmit() provides.
Why would we need such duplication? The more that there everything up from
PAGE_SIZE is permitted, while you require at least 16k. If you want to add
useful extra checks, put them there. Bonus would be to also replace the
alloc_xenheap_pages() there (unless there's a hidden reason for it to
remain physically contiguous).

Jan