[bug report] crypto: sun8i-ss

* [bug report] crypto: sun8i-ss - rework handling of IV
@ 2022-08-25 16:08 Dan Carpenter
  2022-08-26 13:00 ` LABBE Corentin
  0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-08-25 16:08 UTC (permalink / raw)
  To: clabbe; +Cc: linux-crypto, linux-sunxi

Hello Corentin Labbe,

The patch 359e893e8af4: "crypto: sun8i-ss - rework handling of IV"
from May 2, 2022, leads to the following Smatch static checker
warning:

    drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c:146 sun8i_ss_setup_ivs()
    warn: 'a' is not a DMA mapping error

    drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c:213 sun8i_ss_cipher()
    warn: 'rctx->p_key' is not a DMA mapping error

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
    115 static int sun8i_ss_setup_ivs(struct skcipher_request *areq)
    116 {
    117         struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
    118         struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
    119         struct sun8i_ss_dev *ss = op->ss;
    120         struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq);
    121         struct scatterlist *sg = areq->src;
    122         unsigned int todo, offset;
    123         unsigned int len = areq->cryptlen;
    124         unsigned int ivsize = crypto_skcipher_ivsize(tfm);
    125         struct sun8i_ss_flow *sf = &ss->flows[rctx->flow];
    126         int i = 0;
    127         u32 a;

This needs to be a dma_addr_t a;

    128         int err;
    129 
    130         rctx->ivlen = ivsize;
    131         if (rctx->op_dir & SS_DECRYPTION) {
    132                 offset = areq->cryptlen - ivsize;
    133                 scatterwalk_map_and_copy(sf->biv, areq->src, offset,
    134                                          ivsize, 0);
    135         }
    136 
    137         /* we need to copy all IVs from source in case DMA is bi-directionnal */
    138         while (sg && len) {
    139                 if (sg_dma_len(sg) == 0) {
    140                         sg = sg_next(sg);
    141                         continue;
    142                 }
    143                 if (i == 0)
    144                         memcpy(sf->iv[0], areq->iv, ivsize);
    145                 a = dma_map_single(ss->dev, sf->iv[i], ivsize, DMA_TO_DEVICE);
--> 146                 if (dma_mapping_error(ss->dev, a)) {

This can't be true because of the 32/63 bit bug.

    147                         memzero_explicit(sf->iv[i], ivsize);
    148                         dev_err(ss->dev, "Cannot DMA MAP IV\n");
    149                         err = -EFAULT;
    150                         goto dma_iv_error;
    151                 }
    152                 rctx->p_iv[i] = a;

But then only 32 bits are used later in the driver in ->p_iv[].  So it's
more complicated than I thought.

    153                 /* we need to setup all others IVs only in the decrypt way */
    154                 if (rctx->op_dir & SS_ENCRYPTION)
    155                         return 0;
    156                 todo = min(len, sg_dma_len(sg));
    157                 len -= todo;
    158                 i++;
    159                 if (i < MAX_SG) {
    160                         offset = sg->length - ivsize;
    161                         scatterwalk_map_and_copy(sf->iv[i], sg, offset, ivsize, 0);
    162                 }
    163                 rctx->niv = i;
    164                 sg = sg_next(sg);
    165         }
    166 
    167         return 0;
    168 dma_iv_error:
    169         i--;
    170         while (i >= 0) {
    171                 dma_unmap_single(ss->dev, rctx->p_iv[i], ivsize, DMA_TO_DEVICE);
    172                 memzero_explicit(sf->iv[i], ivsize);
    173                 i--;
    174         }
    175         return err;
    176 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 2+ messages in thread