Re: [net-next v36] mctp pcc: Implement MCTP over PCC Transport

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
mctp pcc: Implement MCTP over PCC Transport

This commit implements a network driver for the Management Component
Transport Protocol (MCTP) over Platform Communication Channel (PCC).
It creates MCTP network devices via ACPI that communicate through shared
memory and mailbox mechanisms.

Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html

> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index cf325ab0b1ef..77cd4091050c 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -47,6 +47,19 @@ config MCTP_TRANSPORT_I3C
>  	  A MCTP protocol network device is created for each I3C bus
>  	  having a "mctp-controller" devicetree property.
>
> +config MCTP_TRANSPORT_PCC
> +	tristate "MCTP PCC transport"
> +	depends on ACPI

Can MCTP_TRANSPORT_PCC be selected when CONFIG_PCC is disabled?

The Kconfig depends only on ACPI but uses pcc_mbox_request_channel() and
pcc_mbox_free_channel() from the PCC mailbox driver (drivers/mailbox/pcc.c).
When CONFIG_PCC is not set, include/acpi/pcc.h provides stub functions
that make pcc_mbox_request_channel() unconditionally return ERR_PTR(-ENODEV).

This means the driver compiles but initialize_MTU() fails, ndo_open() fails,
and no MCTP-over-PCC communication is possible.

> +	help
> +	  Provides a driver to access MCTP devices over PCC transport,
> +	  A MCTP protocol network device is created via ACPI for each
> +	  entry in the DSDT/SSDT that matches the identifier. The Platform
> +	  communication channels are selected from the corresponding
> +	  entries in the PCCT.
> +
> +	  Say y here if you need to connect to MCTP endpoints over PCC. To
> +	  compile as a module, use m; the module will be called mctp-pcc.

[ ... ]

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..fe08fd041d9c
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c

[ ... ]

> +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> +{
> +	struct acpi_pcct_ext_pcc_shared_memory pcc_header;
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct mctp_pcc_mailbox *inbox;
> +	struct mctp_skb_cb *cb;
> +	struct sk_buff *skb;
> +	int size;
> +
> +	mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> +	inbox = &mctp_pcc_ndev->inbox;
> +	memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
> +
> +	if (pcc_header.length <= 0)  {
> +		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +		return;
> +	}
> +
> +	size = pcc_header.length - sizeof(pcc_header.command) + sizeof(pcc_header);
> +	if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0) {
> +		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +		return;
> +	}
> +
> +	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +	if (!skb) {
> +		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +		return;
> +	}
> +	skb_put(skb, size);
> +	skb->protocol = htons(ETH_P_MCTP);
> +	memcpy_fromio(skb->data, inbox->chan->shmem, size);

Can pcc_header.length exceed inbox->chan->shmem_size?

The pcc_header is read from shared memory controlled by the remote endpoint.
The only validation is pcc_header.length <= 0. The size is calculated as:

    size = pcc_header.length - sizeof(pcc_header.command) + sizeof(pcc_header)
         = pcc_header.length + 12

If the remote endpoint sets pcc_header.length to a value larger than
shmem_size, memcpy_fromio() reads beyond the ioremapped region.

> +	dev_dstats_rx_add(mctp_pcc_ndev->ndev, size);
> +	skb_pull(skb, sizeof(pcc_header));
> +	skb_reset_mac_header(skb);
> +	skb_reset_network_header(skb);
> +	cb = __mctp_cb(skb);
> +	cb->halen = 0;
> +	netif_rx(skb);
> +}

[ ... ]

> +static int mctp_pcc_ndo_stop(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +
> +	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +	pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
> +	return 0;
> +}

Does mctp_pcc_ndo_stop() leak SKBs queued in the mailbox?

Looking at mbox_free_channel() in drivers/mailbox/mailbox.c, the
documentation states "The queued TX requests are simply aborted, no
callbacks are made." The implementation sets chan->cl = NULL and
chan->active_req = NULL but does not free the SKBs stored in
chan->msg_data[].

The mailbox ring buffer can hold up to MBOX_TX_QUEUE_LEN (20) entries.
When the network interface is stopped, any SKBs queued there are leaked
because:

1. The networking stack already returned NETDEV_TX_OK, so it won't free them
2. tx_done will never be called since chan->cl is NULL

The commit message claims "Stopping the interface also frees any packets
that are cached in the mailbox ringbuffer" but this doesn't appear to
happen.

[ ... ]

> +static int initialize_MTU(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct mctp_pcc_mailbox *outbox;
> +	struct pcc_mbox_chan *pchan;
> +	int mctp_pcc_mtu;
> +
> +	mctp_pcc_mtu = MCTP_MIN_MTU;
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	outbox = &mctp_pcc_ndev->outbox;
> +	pchan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +	if (IS_ERR(pchan))
> +		return -1;
> +
> +	mctp_pcc_mtu = pchan->shmem_size;
> +	pcc_mbox_free_channel(pchan);
> +
> +	mctp_pcc_mtu = mctp_pcc_mtu - sizeof(struct acpi_pcct_ext_pcc_shared_memory);

What happens if pchan->shmem_size is smaller than 16 bytes?

The function subtracts sizeof(struct acpi_pcct_ext_pcc_shared_memory)
(16 bytes) from pchan->shmem_size (u64) through the local int variable
mctp_pcc_mtu. If shmem_size is smaller than 16, the subtraction produces
a negative int which wraps when assigned to ndev->max_mtu (unsigned int),
resulting in a value near UINT_MAX.

When packets are transmitted, mctp_pcc_tx_prepare() calls:

    memcpy_toio(outbox->chan->shmem, skb->data, skb->len)

where skb->len can far exceed the actual mapped region (only shmem_size
bytes were ioremapped), causing writes beyond the mapped region.

> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	return 0;
> +}

[ ... ]
-- 
pw-bot: cr