Re: [PATCH 11/24] serial: 8250: cap OxSemi Tornado UART count against BAR size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: returning 0 in pci_oxsemi_tornado_init() bypasses BAR size cap
--

commit 26f7484037e0f4875aba7039d37fa5afbc65ceb4
Author: Joshua Rogers <linux@joshua.hu>

serial: 8250: cap OxSemi Tornado UART count against BAR size

This commit caps the device-reported UART count in pci_oxsemi_tornado_init()
against the size of BAR0 based on the Tornado register layout.

> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 1dda63be6b893..9919504334bb7 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -1083,6 +1083,7 @@ static int pci_oxsemi_tornado_init(struct pci_dev *dev)
>  	u8 __iomem *p;
>  	unsigned long deviceID;
>  	unsigned int  number_uarts = 0;
> +	resource_size_t bar_len;
>  
>  	if (!pci_oxsemi_tornado_p(dev))
>  		return 0;
> @@ -1095,6 +1096,15 @@ static int pci_oxsemi_tornado_init(struct pci_dev *dev)
>  	/* Tornado device */
>  	if (deviceID == 0x07000200) {
>  		number_uarts = ioread8(p + 4);
> +
> +		/* Cap against BAR0 size (Tornado layout: 0x1000 + n * 0x200) */
> +		bar_len = pci_resource_len(dev, 0);
> +		if (bar_len >= 0x1000)
> +			number_uarts = min_t(unsigned int, number_uarts,
> +					     (bar_len - 0x1000) / 0x200);
> +		else
> +			number_uarts = 0;

[Severity: High]
If bar_len is smaller than 0x1200 (either because bar_len < 0x1000 or because
0x1000 <= bar_len < 0x1200 where (bar_len - 0x1000) / 0x200 evaluates to 0),
or if ioread8(p + 4) returns 0, pci_oxsemi_tornado_init() sets number_uarts
to 0 and returns 0.

However, in the caller pciserial_init_ports() in
drivers/tty/serial/8250/8250_pci.c, a return value of 0 from quirk->init()
means "use board->num_ports" rather than 0 ports:

	if (quirk->init) {
		rc = quirk->init(dev);
		if (rc < 0) {
			priv = ERR_PTR(rc);
			goto err_out;
		}
		if (rc)
			nr_ports = rc;
	}

Because the Tornado board entries (such as pbn_oxsemi_1_15625000 through
pbn_oxsemi_8_15625000 in pci_boards[]) do not set FL_REGION_SZ_CAP in
board->flags, pci_default_setup() does not check idx against maxnr:

	if (board->flags & FL_REGION_SZ_CAP && idx >= maxnr)
		return 1;

	return setup_port(priv, port, bar, offset, board->reg_shift);

When pci_oxsemi_tornado_init() returns 0 on an undersized BAR0, won't
pciserial_init_ports() fall back to board->num_ports (up to 8 ports) and map
only bar_len bytes in setup_port() -> serial8250_pci_setup_port(), causing
out-of-bounds MMIO accesses at offsets 0x1000 + idx * 0x200 past the end of
BAR0?

Should pci_oxsemi_tornado_init() return a negative error code (such as
-ENODEV or -EINVAL) when number_uarts is 0 for a Tornado device so that
pciserial_init_ports() aborts probing instead of falling back to
board->num_ports?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-tty-llm-v1-0-99b0202ae546@linuxfoundation.org?part=11