diff mbox series

[v2,4/7] crypto: sun4i-ss: handle BigEndian for cipher

Message ID 1600627038-40000-5-git-send-email-clabbe@baylibre.com (mailing list archive)
State New, archived
Headers show
Series crypto: sun4i-ss: prevent always fallback for ciphers | expand

Commit Message

Corentin LABBE Sept. 20, 2020, 6:37 p.m. UTC 
Ciphers produce invalid results on BE.
Key and IV need to be written in LE.

Fixes: 6298e948215f2 ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
Cc: <stable@vger.kernel.org>
Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

kernel test robot Sept. 23, 2020, 1:33 p.m. UTC | #1 
Hi Corentin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on sunxi/sunxi/for-next]
[also build test WARNING on cryptodev/master crypto/master soc/for-next v5.9-rc6 next-20200923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Corentin-Labbe/crypto-sun4i-ss-prevent-always-fallback-for-ciphers/20200921-023818
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
config: arm64-randconfig-s031-20200923 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-201-g24bdaac6-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: cast from restricted __le32
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 [usertype] @@
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse:     expected unsigned int [usertype] val
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse:     got restricted __le32 [usertype]
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: cast from restricted __le32
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: cast from restricted __le32
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: cast from restricted __le32
>> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:56:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 [usertype] @@
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse:     expected unsigned int [usertype] val
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse:     got restricted __le32 [usertype]
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:61:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 [usertype] @@
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse:     expected unsigned int [usertype] val
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse:     got restricted __le32 [usertype]
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:229:17: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 [usertype] @@
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse:     expected unsigned int [usertype] val
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse:     got restricted __le32 [usertype]
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: cast from restricted __le32
   drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c:234:25: sparse: sparse: cast from restricted __le32

# https://github.com/0day-ci/linux/commit/e7d9839f5afd2cd58a23dd28ee38538dbfef7609
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Corentin-Labbe/crypto-sun4i-ss-prevent-always-fallback-for-ciphers/20200921-023818
git checkout e7d9839f5afd2cd58a23dd28ee38538dbfef7609
vim +56 drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c

    14	
    15	static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
    16	{
    17		struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
    18		struct sun4i_tfm_ctx *op = crypto_skcipher_ctx(tfm);
    19		struct sun4i_ss_ctx *ss = op->ss;
    20		unsigned int ivsize = crypto_skcipher_ivsize(tfm);
    21		struct sun4i_cipher_req_ctx *ctx = skcipher_request_ctx(areq);
    22		u32 mode = ctx->mode;
    23		void *backup_iv = NULL;
    24		/* when activating SS, the default FIFO space is SS_RX_DEFAULT(32) */
    25		u32 rx_cnt = SS_RX_DEFAULT;
    26		u32 tx_cnt = 0;
    27		u32 spaces;
    28		u32 v;
    29		int err = 0;
    30		unsigned int i;
    31		unsigned int ileft = areq->cryptlen;
    32		unsigned int oleft = areq->cryptlen;
    33		unsigned int todo;
    34		struct sg_mapping_iter mi, mo;
    35		unsigned int oi, oo; /* offset for in and out */
    36		unsigned long flags;
    37	
    38		if (!areq->cryptlen)
    39			return 0;
    40	
    41		if (!areq->src || !areq->dst) {
    42			dev_err_ratelimited(ss->dev, "ERROR: Some SGs are NULL\n");
    43			return -EINVAL;
    44		}
    45	
    46		if (areq->iv && ivsize > 0 && mode & SS_DECRYPTION) {
    47			backup_iv = kzalloc(ivsize, GFP_KERNEL);
    48			if (!backup_iv)
    49				return -ENOMEM;
    50			scatterwalk_map_and_copy(backup_iv, areq->src, areq->cryptlen - ivsize, ivsize, 0);
    51		}
    52	
    53		spin_lock_irqsave(&ss->slock, flags);
    54	
    55		for (i = 0; i < op->keylen / 4; i++)
  > 56			writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);
    57	
    58		if (areq->iv) {
    59			for (i = 0; i < 4 && i < ivsize / 4; i++) {
    60				v = *(u32 *)(areq->iv + i * 4);
    61				writel(cpu_to_le32(v), ss->base + SS_IV0 + i * 4);
    62			}
    63		}
    64		writel(mode, ss->base + SS_CTL);
    65	
    66		sg_miter_start(&mi, areq->src, sg_nents(areq->src),
    67			       SG_MITER_FROM_SG | SG_MITER_ATOMIC);
    68		sg_miter_start(&mo, areq->dst, sg_nents(areq->dst),
    69			       SG_MITER_TO_SG | SG_MITER_ATOMIC);
    70		sg_miter_next(&mi);
    71		sg_miter_next(&mo);
    72		if (!mi.addr || !mo.addr) {
    73			dev_err_ratelimited(ss->dev, "ERROR: sg_miter return null\n");
    74			err = -EINVAL;
    75			goto release_ss;
    76		}
    77	
    78		ileft = areq->cryptlen / 4;
    79		oleft = areq->cryptlen / 4;
    80		oi = 0;
    81		oo = 0;
    82		do {
    83			todo = min(rx_cnt, ileft);
    84			todo = min_t(size_t, todo, (mi.length - oi) / 4);
    85			if (todo) {
    86				ileft -= todo;
    87				writesl(ss->base + SS_RXFIFO, mi.addr + oi, todo);
    88				oi += todo * 4;
    89			}
    90			if (oi == mi.length) {
    91				sg_miter_next(&mi);
    92				oi = 0;
    93			}
    94	
    95			spaces = readl(ss->base + SS_FCSR);
    96			rx_cnt = SS_RXFIFO_SPACES(spaces);
    97			tx_cnt = SS_TXFIFO_SPACES(spaces);
    98	
    99			todo = min(tx_cnt, oleft);
   100			todo = min_t(size_t, todo, (mo.length - oo) / 4);
   101			if (todo) {
   102				oleft -= todo;
   103				readsl(ss->base + SS_TXFIFO, mo.addr + oo, todo);
   104				oo += todo * 4;
   105			}
   106			if (oo == mo.length) {
   107				sg_miter_next(&mo);
   108				oo = 0;
   109			}
   110		} while (oleft);
   111	
   112		if (areq->iv) {
   113			if (mode & SS_DECRYPTION) {
   114				memcpy(areq->iv, backup_iv, ivsize);
   115				kfree_sensitive(backup_iv);
   116			} else {
   117				scatterwalk_map_and_copy(areq->iv, areq->dst, areq->cryptlen - ivsize,
   118							 ivsize, 0);
   119			}
   120		}
   121	
   122	release_ss:
   123		sg_miter_stop(&mi);
   124		sg_miter_stop(&mo);
   125		writel(0, ss->base + SS_CTL);
   126		spin_unlock_irqrestore(&ss->slock, flags);
   127		return err;
   128	}
   129	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Arnd Bergmann Sept. 23, 2020, 2 p.m. UTC | #2 
On Sun, Sep 20, 2020 at 8:37 PM Corentin Labbe <clabbe@baylibre.com> wrote:
>
> Ciphers produce invalid results on BE.
> Key and IV need to be written in LE.
>
> Fixes: 6298e948215f2 ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> index c6c25204780d..a05889745097 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> @@ -52,13 +52,13 @@ static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
>
>         spin_lock_irqsave(&ss->slock, flags);
>
> -       for (i = 0; i < op->keylen; i += 4)
> -               writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
> +       for (i = 0; i < op->keylen / 4; i++)
> +               writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);

I suspect what you actually want here is writesl() in place of the
loop. This skips the byteswap on big-endian, rather than swapping
each word twice.

The point is that this register seems to act as a FIFO for a byte-stream
rather than a 32-bit fixed-endian register.

     Arnd
Corentin LABBE Sept. 23, 2020, 6:06 p.m. UTC | #3 
On Wed, Sep 23, 2020 at 04:00:32PM +0200, Arnd Bergmann wrote:
> On Sun, Sep 20, 2020 at 8:37 PM Corentin Labbe <clabbe@baylibre.com> wrote:
> >
> > Ciphers produce invalid results on BE.
> > Key and IV need to be written in LE.
> >
> > Fixes: 6298e948215f2 ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> >  drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > index c6c25204780d..a05889745097 100644
> > --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > @@ -52,13 +52,13 @@ static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
> >
> >         spin_lock_irqsave(&ss->slock, flags);
> >
> > -       for (i = 0; i < op->keylen; i += 4)
> > -               writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
> > +       for (i = 0; i < op->keylen / 4; i++)
> > +               writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);
> 
> I suspect what you actually want here is writesl() in place of the
> loop. This skips the byteswap on big-endian, rather than swapping
> each word twice.
> 
> The point is that this register seems to act as a FIFO for a byte-stream
> rather than a 32-bit fixed-endian register.
> 
>      Arnd

Thanks, using writesl() fixes the warning, but I need to keep the loop since the register is different each time.
Or does it is better to use directly __raw_writel() ?
Arnd Bergmann Sept. 23, 2020, 6:59 p.m. UTC | #4 
On Wed, Sep 23, 2020 at 8:08 PM LABBE Corentin <clabbe@baylibre.com> wrote:
> On Wed, Sep 23, 2020 at 04:00:32PM +0200, Arnd Bergmann wrote:
> > On Sun, Sep 20, 2020 at 8:37 PM Corentin Labbe <clabbe@baylibre.com> wrote:
> > > diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > > index c6c25204780d..a05889745097 100644
> > > --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > > +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> > > @@ -52,13 +52,13 @@ static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
> > >
> > >         spin_lock_irqsave(&ss->slock, flags);
> > >
> > > -       for (i = 0; i < op->keylen; i += 4)
> > > -               writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
> > > +       for (i = 0; i < op->keylen / 4; i++)
> > > +               writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);
> >
> > I suspect what you actually want here is writesl() in place of the
> > loop. This skips the byteswap on big-endian, rather than swapping
> > each word twice.
> >
> > The point is that this register seems to act as a FIFO for a byte-stream
> > rather than a 32-bit fixed-endian register.
>
> Thanks, using writesl() fixes the warning, but I need to keep the loop
> since the register is different each time.

Ah, I see. I thought we had an interface for that as well, but I can't
find it now. I see memcpy_toio32() in one driver, but that implementation
appears to be wrong here (and probably also wrong for the machine
it was meant for)

There is the regular memcpy_toio(), but on big-endian Arm that
turns into a per-byte copy, which might either not work on your
hardware or be too slow.

There is also __iowrite32_copy(), which is not what I had remembered
but does seem to do what you want here.

> Or does it is better to use directly __raw_writel() ?

__raw_writel() is not very portable, so I would avoid that in normal
device drivers even when you only run them on specific hardware.

      Arnd
diff mbox series

Patch

diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
index c6c25204780d..a05889745097 100644
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
@@ -52,13 +52,13 @@  static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
 
 	spin_lock_irqsave(&ss->slock, flags);
 
-	for (i = 0; i < op->keylen; i += 4)
-		writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
+	for (i = 0; i < op->keylen / 4; i++)
+		writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);
 
 	if (areq->iv) {
 		for (i = 0; i < 4 && i < ivsize / 4; i++) {
 			v = *(u32 *)(areq->iv + i * 4);
-			writel(v, ss->base + SS_IV0 + i * 4);
+			writel(cpu_to_le32(v), ss->base + SS_IV0 + i * 4);
 		}
 	}
 	writel(mode, ss->base + SS_CTL);
@@ -225,13 +225,13 @@  static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
 
 	spin_lock_irqsave(&ss->slock, flags);
 
-	for (i = 0; i < op->keylen; i += 4)
-		writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
+	for (i = 0; i < op->keylen / 4; i++)
+		writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);
 
 	if (areq->iv) {
 		for (i = 0; i < 4 && i < ivsize / 4; i++) {
 			v = *(u32 *)(areq->iv + i * 4);
-			writel(v, ss->base + SS_IV0 + i * 4);
+			writel(cpu_to_le32(v), ss->base + SS_IV0 + i * 4);
 		}
 	}
 	writel(mode, ss->base + SS_CTL);