| Message ID | 20230321140357.24094-5-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
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
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@amd.com> 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
On 29.03.2023 16:35, Ayan Kumar Halder wrote: > Please let me know if the below patch looks fine. Apart from the comments below there may be formatting issues, which I can't sensibly comment on when the patch was mangled by your mailer anyway. (Which in turn is why it is generally better to properly send a new version, rather than replying with kind-of-a-new-version on an earlier thread.) Additionally, up front: I'm sorry for the extra requests, but I'm afraid to sensibly make the changes you want to make some things need sorting first, to avoid extending pre-existing clumsiness. This is irrespective of the present state of things clearly not being your fault. > @@ -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; > + } Further down the function returns -1, so here you assume EINVAL != 1. Such assumptions (and mixing of value spaces) is generally not a good idea. Since there are other issues (see below), maybe you really want to add a prereq patch addressing those? That would include changing the "return -1" to either "return 1" or making it use some sensible and properly distinguishable errno value. > @@ -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; With skip_amt != 0 the function presently can only return 0. You're therefore converting pre-existing dead code to another form of dead code. Otoh (and as, I think, previously indicated) ... > + > 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; ... the equivalent of this in parse_namevalue_pairs() not checking the return value is bogus. But it is further bogus that the case where skip_amt has passed 1 for it sets dev_set to true unconditionally, i.e. even when no device was found. IOW I also question the correctness of the final "return 0" in pci_uart_config(). I looks to me as if this wants to be a skip_amt-independent "return -ENODEV". skip_amt would only control whether uart->io_base is restored before returning (leaving aside the question of why that is). Jan
Hi Jan, On 30/03/2023 07:55, Jan Beulich wrote: > On 29.03.2023 16:35, Ayan Kumar Halder wrote: >> Please let me know if the below patch looks fine. > Apart from the comments below there may be formatting issues, which > I can't sensibly comment on when the patch was mangled by your mailer > anyway. (Which in turn is why it is generally better to properly send > a new version, rather than replying with kind-of-a-new-version on an > earlier thread.) > > Additionally, up front: I'm sorry for the extra requests, but I'm > afraid to sensibly make the changes you want to make some things need > sorting first, to avoid extending pre-existing clumsiness. This is > irrespective of the present state of things clearly not being your > fault. > >> @@ -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; >> + } > Further down the function returns -1, so here you assume EINVAL != 1. > Such assumptions (and mixing of value spaces) is generally not a good > idea. Since there are other issues (see below), maybe you really want > to add a prereq patch addressing those? That would include changing the > "return -1" to either "return 1" or making it use some sensible and > properly distinguishable errno value. > >> @@ -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; > With skip_amt != 0 the function presently can only return 0. You're > therefore converting pre-existing dead code to another form of dead > code. Otoh (and as, I think, previously indicated) ... > >> + >> 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; > ... the equivalent of this in parse_namevalue_pairs() not checking > the return value is bogus. But it is further bogus that the case > where skip_amt has passed 1 for it sets dev_set to true > unconditionally, i.e. even when no device was found. IOW I also > question the correctness of the final "return 0" in pci_uart_config(). > I looks to me as if this wants to be a skip_amt-independent > "return -ENODEV". skip_amt would only control whether uart->io_base is > restored before returning (leaving aside the question of why that is). I have sent out a patch to fix the return logic pci_uart_config() [PATCH v1] xen/drivers: ns16550: Fix the return logic for pci_uart_config() Let me know if I understood you correctly. - Ayan > > Jan
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 092f6b9c4b..2e8a9cfb24 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 */ @@ -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 ) + 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 +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) | + (bar & PCI_BASE_ADDRESS_MEM_MASK); + + /* Truncation detected while converting to paddr_t */ + if ( (pci_uart_io_base >> (PADDR_BITS - 1)) > 1 ) + PARSE_ERR_RET("Truncation detected for io_base address"); + + uart->io_base = pci_uart_io_base; } /* IO based */ else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) ) @@ -1456,13 +1470,6 @@ static enum __init serial_param_type get_token(char *token, char **value) return; \ } while ( 0 ) -#define PARSE_ERR_RET(_f, _a...) \ - do { \ - printk( "ERROR: " _f "\n" , ## _a ); \ - return false; \ - } while ( 0 ) - - static bool __init parse_positional(struct ns16550 *uart, char **str) { int baud; @@ -1532,7 +1539,15 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) 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_BITS - 1)) > 1 ) + PARSE_ERR_RET("Truncation detected for uart_io_base address"); + + uart->io_base = uart_io_base; } }
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. Also moved the definition of PARSE_ERR_RET before its first usage. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from - v1 - NA v2 - 1. Extracted the patch from "[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size" into a separate patch of its own. v3 - 1. Reduced the scope of pci_uart_io_base and uart_io_base definitions. 2. Instead of crashing, invoke PARSE_ERR_RET(). 3. Moved PARSE_ERR_RET() so that it is defined before its first use. xen/drivers/char/ns16550.c | 41 ++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-)