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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CH2PR15MB36863AED8F20F62F029C0CFAA3B00@CH2PR15MB3686.namprd15.prod.outlook.com>
Date:   Thu, 12 Sep 2019 19:45:40 +0000
From:   Ben Wei <benwei@...com>
To:     "Justin.Lee1@...l.com" <Justin.Lee1@...l.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "sam@...dozajonas.com" <sam@...dozajonas.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ben Wei <benwei@...com>
Subject: RE: [PATCH net-next] net/ncsi: support unaligned payload size in
 NC-SI cmd handler

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@...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