Re: [PATCH 09/11] NTB/msi: Require a singleton memory window

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Error Handling Bug: incorrect memory window index cleared on rollback
- [High] Integer Underflow / Out-of-Bounds Read: Negative index passed to hardware driver
--

--- Patch [9]: [PATCH 09/11] NTB/msi: Require a singleton memory window ---
commit ed42871f4aa60b4cc0fd8192c14055c9e382384b
Author: Koichiro Den <den@valinux.co.jp>

    NTB/msi: Require a singleton memory window
    
    This commit adds a check in ntb_msi_init() to reject NTB MSI layouts
    where the outbound memory window does not form a singleton translation
    group. This ensures the transport driver can retain non-singleton groups
    for data use.

> diff --git a/drivers/ntb/msi.c b/drivers/ntb/msi.c
> index 6817d504c12ac..029edcf400784 100644
> --- a/drivers/ntb/msi.c
> +++ b/drivers/ntb/msi.c
[ ... ]
> @@ -42,6 +44,21 @@ int ntb_msi_init(struct ntb_dev *ntb,
>  	if (peers <= 0)
>  		return -EINVAL;
>  
> +	if (ntb->ops->mw_get_trans_group) {
> +		for (i = 0; i < peers; i++) {
> +			peer_widx = ntb_peer_highest_mw_idx(ntb, i);
> +			if (peer_widx < 0)
> +				return peer_widx;
> +
> +			ret = ntb_mw_get_trans_group(ntb, i, peer_widx, &first,
> +						     &count);
> +			if (ret)
> +				return ret;
> +			if (first != peer_widx || count != 1)
> +				return -EOPNOTSUPP;
> +		}
> +	}
> +
>  	ntb->msi = devm_kzalloc(&ntb->dev, struct_size(ntb->msi, peer_mws, peers),
>  				GFP_KERNEL);
>  	if (!ntb->msi)

[Severity: High]
This is a pre-existing issue, but does this code properly prevent a negative
index from being passed to the hardware driver?

Further down in drivers/ntb/msi.c:ntb_msi_init():

	for (i = 0; i < peers; i++) {
		peer_widx = ntb_peer_mw_count(ntb) - 1 - i;

		ret = ntb_peer_mw_get_addr(ntb, peer_widx, &mw_phys_addr,
					   &mw_size);

If the hardware exposes fewer outbound memory windows (ntb_peer_mw_count(ntb))
than there are peers (ntb_peer_port_count(ntb)), won't the subtraction
underflow and make peer_widx negative?

Hardware drivers might fail to validate this negative index, resulting in
out-of-bounds array accesses and likely causing a kernel panic during device
probe.

[Severity: High]
This is also a pre-existing issue, but does the error handling in
ntb_msi_setup_mws() clear the correct memory window index on rollback?

In drivers/ntb/msi.c:ntb_msi_setup_mws():

error_out:
	for (i = 0; i < peer; i++) {
		peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
		if (peer_widx < 0)
			continue;

		ntb_mw_clear_trans(ntb, i, peer_widx);
	}

The unroll loop queries ntb_peer_highest_mw_idx(ntb, peer) using the failing
peer index instead of the loop iterator i.

If peers have different memory window counts or indices, won't this leave an
active memory window mapping on the NTB hardware for previously configured
peers, or potentially clear the wrong window?

If the driver later reuses the uncleared memory window for regular data
transfers, remote memory writes could be interpreted as MSIs, leading to
hardware state corruption and spurious interrupts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803180439.1963864-1-den@valinux.co.jp?part=9