[PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[Ben Wei]

Update NC-SI command handler (both standard and OEM) to take into
account of payload paddings in allocating skb (in case of payload
size is not 32-bit aligned).

The checksum field follows payload field, without taking payload
padding into account can cause checksum being truncated, leading to
dropped packets.

Signed-off-by: Ben Wei <benwei@fb.com>
---
 net/ncsi/ncsi-cmd.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 0187e65176c0..42636ed3cf3a 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -213,17 +213,22 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb,
 {
 	struct ncsi_cmd_oem_pkt *cmd;
 	unsigned int len;
+	/* NC-SI spec requires payload to be padded with 0
+	 * to 32-bit boundary before the checksum field.
+	 * Ensure the padding bytes are accounted for in
+	 * skb allocation
+	 */
+	unsigned short payload = ALIGN(nca->payload, 4);
 
 	len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
-	if (nca->payload < 26)
+	if (payload < 26)
 		len += 26;
 	else
-		len += nca->payload;
+		len += payload;
 
 	cmd = skb_put_zero(skb, len);
 	memcpy(&cmd->mfr_id, nca->data, nca->payload);
 	ncsi_cmd_build_header(&cmd->cmd.common, nca);
-
 	return 0;
 }
 
@@ -272,6 +277,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
 	struct net_device *dev = nd->dev;
 	int hlen = LL_RESERVED_SPACE(dev);
 	int tlen = dev->needed_tailroom;
+	int payload;
 	int len = hlen + tlen;
 	struct sk_buff *skb;
 	struct ncsi_request *nr;
@@ -281,14 +287,17 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
 		return NULL;
 
 	/* NCSI command packet has 16-bytes header, payload, 4 bytes checksum.
+	 * Payload needs padding so that the checksum field follwoing payload is
+	 * aligned to 32bit boundary.
 	 * The packet needs padding if its payload is less than 26 bytes to
 	 * meet 64 bytes minimal ethernet frame length.
 	 */
 	len += sizeof(struct ncsi_cmd_pkt_hdr) + 4;
-	if (nca->payload < 26)
+	payload = ALIGN(nca->payload, 4);
+	if (payload < 26)
 		len += 26;
 	else
-		len += nca->payload;
+		len += payload;
 
 	/* Allocate skb */
 	skb = alloc_skb(len, GFP_ATOMIC);
-- 
2.17.1

Re: [PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[David Miller]

From: Ben Wei <benwei@fb.com>
Date: Mon, 2 Sep 2019 02:46:52 +0000

> Update NC-SI command handler (both standard and OEM) to take into
> account of payload paddings in allocating skb (in case of payload
> size is not 32-bit aligned).
> 
> The checksum field follows payload field, without taking payload
> padding into account can cause checksum being truncated, leading to
> dropped packets.
> 
> Signed-off-by: Ben Wei <benwei@fb.com>

If you have to align and add padding, I do not see where you are
clearing out that padding memory to make sure it is initialized.

You do comparisons with 'payload' but make adjustments to 'len'.

The logic is very confusing.

RE: [PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[Ben Wei]

> > Update NC-SI command handler (both standard and OEM) to take into
> > account of payload paddings in allocating skb (in case of payload
> > size is not 32-bit aligned).
> > 
> > The checksum field follows payload field, without taking payload
> > padding into account can cause checksum being truncated, leading to
> > dropped packets.
> > 
> > Signed-off-by: Ben Wei <benwei@fb.com>
>
> If you have to align and add padding, I do not see where you are
> clearing out that padding memory to make sure it is initialized.
>
> You do comparisons with 'payload' but make adjustments to 'len'.
>
> The logic is very confusing.

Yes let me clarify a bit. 

In the code 'payload' is the exact NC-SI payload length, which goes into NC-SI packet header
and needs to be actual unpadded payload length.

'len' is used to allocate total NC-SI packet buffer (include padding). 

The original calculation of 'len' was done by summing up NCSI header + payload + checksum,
without taking into account of possible padding, e.g.

        len += sizeof(struct ncsi_cmd_pkt_hdr) + 4;  /* 4 is the checksum size */
       if (nca->payload < 26)
                len += 26;
        else
               len += nca->payload;
        /* Allocate skb */
        skb = alloc_skb(len, GFP_ATOMIC);

This works today for all standard NC-SI commands (in spec v1.1) because all standard commands
have payload size < 26, and packet size is then set to minimum of 46 (16 hdr + 26 payload + 4 cksum) bytes.

And mem clearing is done in each of the standard cmd handlers, e.g. 
ncsi_cmd_handler_sp, ncsi_cmd_handler_ae.



The problem occurs if payload >= 26 and is unaligned.  This could happen on some OEM commands,
and I see this happening when we carry PLDM traffic over NC-SI packet. 
(PLDM header being 3 bytes and payload size can be large) 

The skb allocated would be too small, and later when checksum is calculated and written:

	pchecksum = (__be32 *)((void *)h + sizeof(struct ncsi_pkt_hdr) +
		    ALIGN(nca->payload, 4));
	*pchecksum = htonl(checksum);

Part of the checksum would fall outside of our allocated buffer.

PLDM over NC-SI and OEM NC-SI commands are currently handled in

@@ -213,17 +213,22 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb,

So here I ensure the skb allocation takes padding into account, and do the initial mem clearing
to set the padding bytes

+       unsigned short payload = ALIGN(nca->payload, 4);

        len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
-       if (nca->payload < 26)
+       if (payload < 26)
                len += 26;
        else
-               len += nca->payload;
+               len += payload;

        cmd = skb_put_zero(skb, len);
        memcpy(&cmd->mfr_id, nca->data, nca->payload);

So in this patch I updated both standard command handler (in case future spec updates adds
commands with payload >= 26) and OEM/generic command handler to support unaligned payload
size.  

Regards,
-Ben

RE: [PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[Justin.Lee1]

That is right. It is necessary to adjust the len for padding on both places.

Thanks,
Justin


> > > Update NC-SI command handler (both standard and OEM) to take into 
> > > account of payload paddings in allocating skb (in case of payload 
> > > size is not 32-bit aligned).
> > > 
> > > The checksum field follows payload field, without taking payload 
> > > padding into account can cause checksum being truncated, leading to 
> > > dropped packets.
> > > 
> > > Signed-off-by: Ben Wei <benwei@fb.com>
> >
> > If you have to align and add padding, I do not see where you are 
> > clearing out that padding memory to make sure it is initialized.
> >
> > You do comparisons with 'payload' but make adjustments to 'len'.
> >
> > The logic is very confusing.
> 
> Yes let me clarify a bit. 
> 
> In the code 'payload' is the exact NC-SI payload length, which goes into NC-SI packet header and needs to be actual unpadded payload length.
> 
> 'len' is used to allocate total NC-SI packet buffer (include padding). 
> 
> The original calculation of 'len' was done by summing up NCSI header + payload + checksum, without taking into account of possible padding, e.g.
> 
>         len += sizeof(struct ncsi_cmd_pkt_hdr) + 4;  /* 4 is the checksum size */
>        if (nca->payload < 26)
>                 len += 26;
>         else
>                len += nca->payload;
>         /* Allocate skb */
>         skb = alloc_skb(len, GFP_ATOMIC);
> 
> This works today for all standard NC-SI commands (in spec v1.1) because all standard commands have payload size < 26, and packet size is then set to minimum of 46 (16 hdr + 26 payload + 4 cksum) bytes.
> 
> And mem clearing is done in each of the standard cmd handlers, e.g. 
> ncsi_cmd_handler_sp, ncsi_cmd_handler_ae.
> 
> 
> 
> The problem occurs if payload >= 26 and is unaligned.  This could happen on some OEM commands, and I see this happening when we carry PLDM traffic over NC-SI packet. 
> (PLDM header being 3 bytes and payload size can be large) 
> 
> The skb allocated would be too small, and later when checksum is calculated and written:
> 
> 	pchecksum = (__be32 *)((void *)h + sizeof(struct ncsi_pkt_hdr) +
> 		    ALIGN(nca->payload, 4));
> 	*pchecksum = htonl(checksum);
> 
> Part of the checksum would fall outside of our allocated buffer.
> 
> PLDM over NC-SI and OEM NC-SI commands are currently handled in
> 
> @@ -213,17 +213,22 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> 
> So here I ensure the skb allocation takes padding into account, and do the initial mem clearing to set the padding bytes
> 
> +       unsigned short payload = ALIGN(nca->payload, 4);
> 
>         len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> -       if (nca->payload < 26)
> +       if (payload < 26)
>                 len += 26;
>         else
> -               len += nca->payload;
> +               len += payload;
> 
>         cmd = skb_put_zero(skb, len);
>         memcpy(&cmd->mfr_id, nca->data, nca->payload);
> 
> So in this patch I updated both standard command handler (in case future spec updates adds commands with payload >= 26) and OEM/generic command handler to support unaligned payload size.  
> 
> Regards,
> -Ben

RE: [PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler

[Ben Wei]

Hi David,

> That is right. It is necessary to adjust the len for padding on both places.
>
> Thanks,
> Justin
>
>
> > > > Update NC-SI command handler (both standard and OEM) to take into 
> > > > account of payload paddings in allocating skb (in case of payload 
> > > > size is not 32-bit aligned).
> > > > 
> > > > The checksum field follows payload field, without taking payload 
> > > > padding into account can cause checksum being truncated, leading to 
> > > > dropped packets.
> > > > 
> > > > Signed-off-by: Ben Wei <benwei@fb.com>
> > >
> > > If you have to align and add padding, I do not see where you are 
> > > clearing out that padding memory to make sure it is initialized.
> > >
> > > You do comparisons with 'payload' but make adjustments to 'len'.
> > >
> > > The logic is very confusing.
> > 
> > Yes let me clarify a bit. 
> > 
> > In the code 'payload' is the exact NC-SI payload length, which goes into NC-SI packet header and needs to be actual unpadded payload length.
> > 
> > 'len' is used to allocate total NC-SI packet buffer (include padding). 
> > 
> > The original calculation of 'len' was done by summing up NCSI header + payload + checksum, without taking into account of possible padding, e.g.
> > 
> >         len += sizeof(struct ncsi_cmd_pkt_hdr) + 4;  /* 4 is the checksum size */
> >        if (nca->payload < 26)
> >                 len += 26;
> >         else
> >                len += nca->payload;
> >         /* Allocate skb */
> >         skb = alloc_skb(len, GFP_ATOMIC);
> > 
> > This works today for all standard NC-SI commands (in spec v1.1) because all standard commands have payload size < 26, and packet size is then set to minimum of 46 (16 hdr + 26 payload + 4 cksum) bytes.
> > 
> > And mem clearing is done in each of the standard cmd handlers, e.g. 
> > ncsi_cmd_handler_sp, ncsi_cmd_handler_ae.
> > 
> > 
> > 
> > The problem occurs if payload >= 26 and is unaligned.  This could happen on some OEM commands, and I see this happening when we carry PLDM traffic over NC-SI packet. 
> > (PLDM header being 3 bytes and payload size can be large) 
> > 
> > The skb allocated would be too small, and later when checksum is calculated and written:
> > 
> > 	pchecksum = (__be32 *)((void *)h + sizeof(struct ncsi_pkt_hdr) +
> > 		    ALIGN(nca->payload, 4));
> > 	*pchecksum = htonl(checksum);
> > 
> > Part of the checksum would fall outside of our allocated buffer.
> > 
> > PLDM over NC-SI and OEM NC-SI commands are currently handled in
> > 
> > @@ -213,17 +213,22 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > 
> > So here I ensure the skb allocation takes padding into account, and do the initial mem clearing to set the padding bytes
> > 
> > +       unsigned short payload = ALIGN(nca->payload, 4);
> > 
> >         len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > -       if (nca->payload < 26)
> > +       if (payload < 26)
> >                 len += 26;
> >         else
> > -               len += nca->payload;
> > +               len += payload;
> > 
> >         cmd = skb_put_zero(skb, len);
> >         memcpy(&cmd->mfr_id, nca->data, nca->payload);
> > 
> > So in this patch I updated both standard command handler (in case future spec updates adds commands with payload >= 26) and OEM/generic command handler to support unaligned payload size.  
> > 


Is this notes sufficient or should I add more comments to the patch?
Basically the issue you raised ('len' vs 'payload') is due to 'len' being used for buffer allocation, which needs padding based on 'payload' (which is exact payload length).

Thanks
-Ben