linux-kernel - Re: [PATCH V12 7/9] cxl/port: Introduce cxl_cdat

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yr0pwgbJJT7HmtDA@iweiny-desk3>
Date:   Wed, 29 Jun 2022 21:42:42 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lukas Wunner <lukas@...ner.de>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Dave Jiang" <dave.jiang@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
        <linux-pci@...r.kernel.org>
Subject: Re: [PATCH V12 7/9] cxl/port: Introduce cxl_cdat_valid()

On Tue, Jun 28, 2022 at 03:49:42PM +0100, Jonathan Cameron wrote:
> On Mon, 27 Jun 2022 21:15:25 -0700
> ira.weiny@...el.com wrote:
> 
> > From: Ira Weiny <ira.weiny@...el.com>
> > 
> > The CDAT data is protected by a checksum and should be the proper
> > length.
> > 
> > Introduce cxl_cdat_valid() to validate the data.  While at it check and
> > store the sequence number.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> > 
> Minor ordering comment.  With that tidied up
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 4bd479ec0253..6d775cc3dca1 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -532,6 +532,40 @@ static int cxl_cdat_get_length(struct device *dev,
> >  	return rc;
> >  }
> >  
> > +static bool cxl_cdat_valid(struct device *dev, struct cxl_cdat *cdat)
> > +{
> > +	u32 *table = cdat->table;
> > +	u8 *data8 = cdat->table;
> > +	u32 length, seq;
> > +	u8 check;
> > +	int i;
> > +
> > +	length = FIELD_GET(CDAT_HEADER_DW0_LENGTH, table[0]);
> > +	if ((length < CDAT_HEADER_LENGTH_BYTES) || (length > cdat->length)) {
> > +		dev_err(dev, "CDAT Invalid length %u (%zu-%zu)\n", length,
> > +			CDAT_HEADER_LENGTH_BYTES, cdat->length);
> > +		return false;
> > +	}
> > +
> > +	for (check = 0, i = 0; i < length; i++)
> > +		check += data8[i];
> > +
> > +	dev_dbg(dev, "CDAT length %u CS %u\n", length, check);
> > +	if (check != 0) {
> > +		dev_err(dev, "CDAT Invalid checksum %u\n", check);
> > +		return false;
> > +	}
> > +
> > +	seq = FIELD_GET(CDAT_HEADER_DW3_SEQUENCE, table[3]);
> > +	/* Store the sequence for now. */
> > +	if (cdat->seq != seq) {
> > +		dev_info(dev, "CDAT seq change %x -> %x\n", cdat->seq, seq);
> > +		cdat->seq = seq;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_cdat_read_table(struct device *dev,
> >  			       struct pci_doe_mb *cdat_mb,
> >  			       struct cxl_cdat *cdat)
> > @@ -579,6 +613,8 @@ static int cxl_cdat_read_table(struct device *dev,
> >  
> >  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
> >  
> > +	if (!rc && !cxl_cdat_valid(dev, cdat))
> > +		return -EIO;
> 
> I'd prefer those handled separately as flow is more readable if error
> handling always out of line.
> 
> 	if (rc)
> 		return rc;

Actually rc is never not 0 here.  :-/

> 
> 	if (!cxl_cdata_valid)
> 		return -EIO;
> 
> 	return 0;

So this is all that is needed.  But the previous patch needs some clean up
to make sense in the series.

Ira