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
Le Thu, Aug 25, 2022 at 07:08:56PM +0300, Dan Carpenter a ?crit :
> 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
Hello
I dont know why I used an u32 and not a dma_addr_t.
I will work on a patch fixing this soon.
Thanks for the report.
>
> 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