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

[Jeremy Kerr <jk@codeconstruct.com.au>]

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@os.amperecomputing.com>

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.

So, for v36, it looks like you have

 * added the CONFIG_PCC dependency
 * altered the RX min length check to include at least the command and
   a MCTP header
 * added a RX max-length check
 * 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?

[...]

> +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().

(but also, should this check also include the size of the pcc header?)

Cheers,


Jeremy