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

[Adam Young]

On 4/1/26 22:16, Jeremy Kerr wrote:
> Hi Adam,
>
> > Implementation of network driver for
> > Management Component Transport Protocol(MCTP)
> > over Platform Communication Channel(PCC)
> >
> > DMTF DSP:0292
> > Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
> >
> > The transport mechanism is called Platform Communication Channels (PCC)
> > is part of the ACPI spec:
> >
> > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html
> >
> > The PCC mechanism is managed via a mailbox implemented at
> > drivers/mailbox/pcc.c
> >
> > MCTP devices are specified via ACPI by entries in DSDT/SSDT and
> > reference channels specified in the PCCT. Messages are sent on a type
> > 3 and received on a type 4 channel.  Communication with other devices
> > use the PCC based doorbell mechanism; a shared memory segment with a
> > corresponding interrupt and a memory register used to trigger remote
> > interrupts.
> >
> > The shared buffer must be at least 68 bytes long as that is the minimum
> > MTU as defined by the MCTP specification.
> >
> > Unlike the existing PCC Type 2 based drivers, the mssg parameter to
> > mbox_send_msg is actively used. The data section of the struct sk_buff
> > that contains the outgoing packet is sent to the mailbox, already
> > properly formatted as a PCC exctended message.
> >
> > If the mailbox ring buffer is full, the driver stops the incoming
> > packet queues until a message has been sent, freeing space in the
> > ring buffer.
> >
> > When the Type 3 channel outbox receives a txdone response interrupt,
> > it consumes the outgoing sk_buff, allowing it to be freed.
> >
> > Bringing up an interface creates the channel between the network driver
> > and the mailbox driver. This enables communication with the remote
> > endpoint, to include the receipt of new messages. Bringing down an
> > interface removes the channel, and no new messages can be delivered.
> > Stopping the interface will leave any packets that are cached in the
> > mailbox ringbuffer. They cannot safely be freed until the PCC mailbox
> > attempts to deliver them and has removed them from the ring buffer.
> >
> > PCC is based on a shared buffer and a set of I/O mapped memory locations
> > that the Spec calls registers.  This mechanism exists regardless of the
> > existence of the driver. If the user has the ability to map these
> > physical location to virtual locations, they have the ability to drive the
> > hardware.  Thus, there is a security aspect to this mechanism that extends
> > beyond the responsibilities of the operating system.
> >
> > If the hardware does not expose the PCC in the ACPI table, this device
> > will never be enabled. Thus it is only an issue on hardware that does
> > support PCC. In that case, it is up to the remote controller to sanitize
> > communication; MCTP will be exposed as a socket interface, and userland
> > can send any crafted packet it wants. It would also be incumbent on
> > the hardware manufacturer to allow the end user to disable MCTP over PCC
> > communication if they did not want to expose it.
> >
> > Signed-off-by: Adam Young <admiyo@xxxxxxxxxxxxxxxxxxxxxx>
> No changelogs anymore? This makes things more difficult to review. Just
> to be clear, when I mentioned changing to a single-patch series, that
> did not mean to throw out the changelog - it's still a key part of
> review.

Sorry about that.  Your list below is accurate.


> So, for v36, it looks like you have
>
>   * added the CONFIG_PCC dependency
Yes.  THis was from Jakub's review
>   * altered the RX min length check to include at least the command and
>     a MCTP header

Yes.  Again, from Jakub's review as well as thinking through what the 
most minimal acceptable message would be.


>   * added a RX max-length check
Correct.  If the max-length is greater than the buffer something is 
misconfigured.
>   * added a minimum MTU check
>   * altered the commit message to reflect PCC mailbox free behaviour.
>
> On the latter, could you expand on what happens on close? Does the PCC
> channel end up calling tx_done() on each pending TX during the channel
> free? I'm not familiar with the PCC paths, but it doesn't look like it
> (or the mailbox core) has a case to deal with this on free.
>
> Maybe I am missing something, but could this leak skbs?

Yes, it could, but since they are held by another subsystem, and there 
is no way to access them, it is safer to leak than to free. The Ring 
Buffer in the Mailbox layer is not accessable from the MCTP Client.  
Additionally, there is no way to force a flush of ring buffer.  This 
should be a vanishingly rare case, I suspect where someone would have to 
deliberately send a bunch of messages and then immediately bring the 
link down.  Bringing the link back up would immediately send the 
remaining skbs and cause them to be free, so they are not frozen in 
perpetuity with no chance of being sent.

Whether the backend could handle this is a different story.

There is a potential for this kind of leak even if we were to transfer 
the data out of SKB:  the two options are to either leave it in the ring 
buffer or risk a use-after-free event.


> [...]
>
> > +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;
> > +       if (pchan->shmem_size < MCTP_MIN_MTU)
> > +               return -1;
> Looks like this would leak the channel? You may want to shift this check
> to after the free_channel().
True.
> (but also, should this check also include the size of the pcc header?)
Yes, good catch.
> Cheers,
>
>
> Jeremy