Received-SPF: pass (google.com: domain of linux-kernel+bounces-178064-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 13 May 2024 22:17:48 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: admiyo@os.amperecomputing.com
Cc: Jeremy Kerr <jk@codeconstruct.com.au>,
	Matt Johnston <matt@codeconstruct.com.au>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport
Message-ID: <48ef7972-4b8f-4027-a2ca-357e53dcdd0f@lunn.ch>
References: <20240513173546.679061-1-admiyo@os.amperecomputing.com>
 <20240513173546.679061-2-admiyo@os.amperecomputing.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240513173546.679061-2-admiyo@os.amperecomputing.com>

> +static struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb,
> +						     void *buffer, int outbox_index)
> +{
> +	struct mctp_pcc_packet *mpp;
> +
> +	mpp = buffer;
> +	writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature);
> +	writel(0x1, &mpp->pcc_header.flags);
> +	memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> +	writel(old_skb->len + SIGNATURE_LENGTH,  &mpp->pcc_header.length);
> +	memcpy_toio(mpp->header_data,    old_skb->data, old_skb->len);
> +	return mpp;
> +}

..

> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	unsigned char *buffer;
> +	struct mctp_pcc_ndev *mpnd;
> +	struct mctp_pcc_packet  *mpp;
> +	unsigned long flags;
> +	int rc;
> +
> +	netif_stop_queue(ndev);
> +	ndev->stats.tx_bytes += skb->len;
> +	mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> +	spin_lock_irqsave(&mpnd->lock, flags);
> +	buffer =  mpnd->pcc_comm_outbox_addr;
> +	mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index);

I don't see any length checks here. How do you know the skb contains
sizeof(struct mctp_pcc_packet)?

> +static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
> +				  struct device *dev, int inbox_index,
> +				  int outbox_index)
> +{
> +	int rc;
> +	int mctp_pcc_mtu;
> +	char name[32];
> +	struct net_device *ndev;
> +	struct mctp_pcc_ndev *mctp_pcc_dev;
> +	struct mctp_pcc_hw_addr physical_link_addr;

Since this is networking code, you should be using reverse christmas
tree for all your functions.

> +	snprintf(name, sizeof(name), "mctpipcc%x", inbox_index);
> +	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, mctp_pcc_setup);

%x is very unusual for network device names.

	Andrew