linux-kernel - RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write

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: <MN2PR20MB29732240A7D7ECE5BA80E474CA690@MN2PR20MB2973.namprd20.prod.outlook.com>
Date:   Mon, 21 Oct 2019 10:23:32 +0000
From:   Pascal Van Leeuwen <pvanleeuwen@...imatrix.com>
To:     "Ben Dooks (Codethink)" <ben.dooks@...ethink.co.uk>,
        "linux-kernel@...ts.codethink.co.uk" 
        <linux-kernel@...ts.codethink.co.uk>
CC:     Antoine Tenart <antoine.tenart@...tlin.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in
 eip197_write_firmware

> -----Original Message-----
> From: Pascal Van Leeuwen
> Sent: Thursday, October 17, 2019 9:46 PM
> To: 'Ben Dooks (Codethink)' <ben.dooks@...ethink.co.uk>; 'linux-kernel@...ts.codethink.co.uk'
> <linux-kernel@...ts.codethink.co.uk>
> Cc: 'Antoine Tenart' <antoine.tenart@...tlin.com>; 'Herbert Xu' <herbert@...dor.apana.org.au>;
> 'David S. Miller' <davem@...emloft.net>; 'linux-crypto@...r.kernel.org' <linux-
> crypto@...r.kernel.org>; 'linux-kernel@...r.kernel.org' <linux-kernel@...r.kernel.org>
> Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> 
> > -----Original Message-----
> > From: Pascal Van Leeuwen
> > Sent: Thursday, October 17, 2019 7:14 PM
> > To: 'Ben Dooks (Codethink)' <ben.dooks@...ethink.co.uk>; linux-
> > kernel@...ts.codethink.co.uk
> > Cc: Antoine Tenart <antoine.tenart@...tlin.com>; Herbert Xu
> > <herbert@...dor.apana.org.au>; David S. Miller <davem@...emloft.net>; linux-
> > crypto@...r.kernel.org; linux-kernel@...r.kernel.org
> > Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> >
> > > -----Original Message-----
> > > From: linux-crypto-owner@...r.kernel.org <linux-crypto-owner@...r.kernel.org> On Behalf
> > Of Ben
> > > Dooks (Codethink)
> > > Sent: Wednesday, October 16, 2019 1:50 PM
> > > To: linux-kernel@...ts.codethink.co.uk
> > > Cc: Ben Dooks (Codethink) <ben.dooks@...ethink.co.uk>; Antoine Tenart
> > > <antoine.tenart@...tlin.com>; Herbert Xu <herbert@...dor.apana.org.au>; David S. Miller
> > > <davem@...emloft.net>; linux-crypto@...r.kernel.org; linux-kernel@...r.kernel.org
> > > Subject: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> > >
> > > In eip197_write_firmware() the firmware buffer is sent using
> > > writel(be32_to_cpu(),,,) this produces a number of warnings.
> > >
> > > Note, should this really be cpu_to_be32()  ?
> > >
> > No, it should certainly not be cpu_to_be32() since the HW itself is most
> > definitely little endian, so that would not make sense to me.
>
Never mind that. While looking into more endianness related sparse issues,
I realised that the HW register access is actually configured to match the
CPU endianness. So the CPU will not need to swap bytes when accessing the
hardware registers.
(Technically, the hardware _is_ little endian but our slave interface
contains byte swapping logic that is enabled for big-endian CPU's ...)

> >
> > Actually, I don't think either solution would be correct on a big-endian
> > CPU. But I don't have any big-endian CPU available to test that theory.
> >
> > What I believe must happen is that the bytes must *always* be swapped
> > here, regardless of the endianness of the CPU. And with a little-endian
> > CPU, be32_to_cpu() coincidentally always does that.
> >
> > Basically, what we need here is: read a dword (32 bits) from the memory
> > subsystem and write it back to the memory subsystem with bytes reversed.
> >
> > Does the kernel have any dedicated function for just always swapping?
> >
> 
> After some more thought on the train home:
> 
> I think the correct construct would be cpu_to_le32(be32_to_cpu(data[i]))
> This would correctly reflect that the data is read from big-endian
> memory and subsequently written to little-endian "memory" (aka the EIP).
> It also fits in nicely with your other changes. Could you work that into
> a patch v2? Then I would ack it (after testing).
> 
Since the HW slave ifc is configured to match the CPU endianness, the
cpu_to_le32() I suggested should NOT be performed and the code is fine as
it was.

> > Anyway: NACK on this patch for now due to this.
> >
Apologies for the mistake and inconvenience. Correction:
Acked-by: Pascal van Leeuwen <pvanleeuwen@...imatrix.com>


> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > >
> > > Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
> > > ---
> > > Cc: Antoine Tenart <antoine.tenart@...tlin.com>
> > > Cc: Herbert Xu <herbert@...dor.apana.org.au>
> > > Cc: "David S. Miller" <davem@...emloft.net>
> > > Cc: linux-crypto@...r.kernel.org
> > > Cc: linux-kernel@...r.kernel.org
> > > ---
> > >  drivers/crypto/inside-secure/safexcel.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-
> > secure/safexcel.c
> > > index 223d1bfdc7e6..dd33f6dda295 100644
> > > --- a/drivers/crypto/inside-secure/safexcel.c
> > > +++ b/drivers/crypto/inside-secure/safexcel.c
> > > @@ -298,13 +298,13 @@ static void eip197_init_firmware(struct safexcel_crypto_priv
> > *priv)
> > >  static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> > >  				  const struct firmware *fw)
> > >  {
> > > -	const u32 *data = (const u32 *)fw->data;
> > > +	const __be32 *data = (const __be32 *)fw->data;
> > >  	int i;
> > >
> > >  	/* Write the firmware */
> > > -	for (i = 0; i < fw->size / sizeof(u32); i++)
> > > +	for (i = 0; i < fw->size / sizeof(__be32); i++)
> > >  		writel(be32_to_cpu(data[i]),
> > > -		       priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
> > > +		       priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(__be32));
> > >
> > >  	/* Exclude final 2 NOPs from size */
> > >  	return i - EIP197_FW_TERMINAL_NOPS;
> > > --
> > > 2.23.0
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > www.insidesecure.com
> 
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com