Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Jan,

On 21/03/2023 14:16, Jan Beulich wrote:
> On 21.03.2023 15:03, Ayan Kumar Halder wrote:
> > @@ -1163,10 +1163,16 @@ static const struct ns16550_config __initconst 
> > uart_config[] =
> >       },
> >   };
> >   
> > +#define PARSE_ERR_RET(_f, _a...)             \
> > +    do {                                     \
> > +        printk( "ERROR: " _f "\n" , ## _a ); \
> > +        return false;                        \
> > +    } while ( 0 )
> You can't really re-use this construct unchanged (and perhaps it's also
> not worth changing for this single use that you need): Note the "return
> false", which ...
>
> >   static int __init
> >   pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> ... for a function returning "int" is equivalent to "return 0", which
> is kind of a success indicator here. Whatever adjustment you make
> needs to be in line with (at least) the two callers checking the
> return value (the other two not doing so is suspicious, but then the
> way the return values are used is somewhat odd, too).
>
> > @@ -1235,6 +1241,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> > unsigned int idx)
> >                   /* MMIO based */
> >                   if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
> >                   {
> > +                    uint64_t pci_uart_io_base;
> > +
> >                       pci_conf_write32(PCI_SBDF(0, b, d, f),
> >                                        PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
> >                       len = pci_conf_read32(PCI_SBDF(0, b, d, f),
> > @@ -1259,8 +1267,14 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> > unsigned int idx)
> >                       else
> >                           size = len & PCI_BASE_ADDRESS_MEM_MASK;
> >   
> > -                    uart->io_base = ((u64)bar_64 << 32) |
> > -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
> > +                    pci_uart_io_base = ((u64)bar_64 << 32) |
> As you touch this code, please be so kind and also switch to using
> uint64_t here.
>
> Also why do you change parse_positional() but not (also)
> parse_namevalue_pairs()?
>
> Jan
Please let me know if the below patch looks fine.

    xen/drivers: ns16550: Use paddr_t for io_base/io_size

    io_base and io_size represent physical addresses. So they should use
    paddr_t (instead of u64).

    However in future, paddr_t may be defined as u32. So when typecasting
    values from u64 to paddr_t, one should always check for any possible
    truncation. If any truncation is discovered, Xen needs to return an
    appropriate an error message for this.

    Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 092f6b9c4b..5c52e7e642 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -42,8 +42,8 @@

 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
-    u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u64 io_size;
+    paddr_t io_base;   /* I/O port or memory-mapped I/O address. */
+    paddr_t io_size;
     int reg_shift; /* Bits to shift register offset by */
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
@@ -1166,7 +1166,7 @@ static const struct ns16550_config __initconst 
uart_config[] =
 static int __init
 pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
-    u64 orig_base = uart->io_base;
+    paddr_t orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;

     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on 
bus 0. */
@@ -1235,6 +1235,8 @@ pci_uart_config(struct ns16550 *uart, bool_t 
skip_amt, unsigned int idx)
                 /* MMIO based */
                 if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
+                    uint64_t pci_uart_io_base;
+
                     pci_conf_write32(PCI_SBDF(0, b, d, f),
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
                     len = pci_conf_read32(PCI_SBDF(0, b, d, f),
@@ -1259,8 +1261,17 @@ pci_uart_config(struct ns16550 *uart, bool_t 
skip_amt, unsigned int idx)
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;

-                    uart->io_base = ((u64)bar_64 << 32) |
-                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                    pci_uart_io_base = ((uint64_t)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+
+                    /* Truncation detected while converting to paddr_t */
+                    if ( pci_uart_io_base != (paddr_t)pci_uart_io_base )
+                    {
+                        printk("ERROR: Truncation detected for io_base 
address");
+                        return -EINVAL;
+                    }
+
+                    uart->io_base = pci_uart_io_base;
                 }
                 /* IO based */
                 else if ( !param->mmio && (bar & 
PCI_BASE_ADDRESS_SPACE_IO) )
@@ -1519,20 +1530,40 @@ static bool __init parse_positional(struct 
ns16550 *uart, char **str)
 #ifdef CONFIG_HAS_PCI
         if ( strncmp(conf, "pci", 3) == 0 )
         {
-            if ( pci_uart_config(uart, 1/* skip AMT */, uart - 
ns16550_com) )
+            int ret;
+
+            ret = pci_uart_config(uart, 1/* skip AMT */, uart - 
ns16550_com);
+
+            if ( ret == -EINVAL )
+                return false;
+            else if ( ret )
                 return true;
+
             conf += 3;
         }
         else if ( strncmp(conf, "amt", 3) == 0 )
         {
-            if ( pci_uart_config(uart, 0, uart - ns16550_com) )
+            int ret = pci_uart_config(uart, 0, uart - ns16550_com);
+
+            if ( ret == -EINVAL )
+                return false;
+            else if ( ret )
                 return true;
+
             conf += 3;
         }
         else
 #endif
         {
-            uart->io_base = simple_strtoull(conf, &conf, 0);
+            uint64_t uart_io_base;
+
+            uart_io_base = simple_strtoull(conf, &conf, 0);
+
+            /* Truncation detected while converting to paddr_t */
+            if ( uart_io_base != (paddr_t)uart_io_base )
+                PARSE_ERR_RET("Truncation detected for uart_io_base 
address");
+
+            uart->io_base = uart_io_base;
         }
     }

@@ -1577,6 +1608,7 @@ static bool __init parse_namevalue_pairs(char 
*str, struct ns16550 *uart)
     char *token, *start = str;
     char *param_value = NULL;
     bool dev_set = false;
+    uint64_t uart_io_base;

     if ( (str == NULL) || (*str == '\0') )
         return true;
@@ -1603,7 +1635,14 @@ static bool __init parse_namevalue_pairs(char 
*str, struct ns16550 *uart)
                        "Can't use io_base with dev=pci or dev=amt 
options\n");
                 break;
             }
-            uart->io_base = simple_strtoull(param_value, NULL, 0);
+

+            uart_io_base = simple_strtoull(param_value, NULL, 0);

+            uart->io_base = uart_io_base;
+            if ( uart_io_base != uart->io_base )
+                PARSE_ERR_RET("Truncation detected for io_base address");
+
             break;

         case irq:


- Ayan