| Message ID | 20210618211411.1167726-1-sean.anderson@seco.com |
|---|---|
| State | Accepted |
| Commit | df6313d707e575a679ada3313358289af24454c0 |
| Headers | show |
| Series | [1/2] crypto: mxs-dcp: Check for DMA mapping errors | expand |
On Fri, Jun 18, 2021 at 05:14:11PM -0400, Sean Anderson wrote: > This uses the sg_miter_*() functions to copy data, instead of doing it > ourselves. Using sg_copy_buffer() would be better, but this way we don't > have to keep traversing the beginning of the scatterlist every time we > do another copy. > > In addition to reducing code size, this fixes the following oops > resulting from failing to kmap the page: Thanks for the patch. Just a minor nit: > @@ -365,25 +364,13 @@ static int mxs_dcp_aes_block_crypt(struct crypto_async_request *arq) > > out_tmp = out_buf; > last_out_len = actx->fill; > - while (dst && actx->fill) { > - if (!split) { > - dst_buf = sg_virt(dst); > - dst_off = 0; > - } > - rem = min(sg_dma_len(dst) - dst_off, > - actx->fill); > - > - memcpy(dst_buf + dst_off, out_tmp, rem); > + > + while (sg_miter_next(&dst_iter) && actx->fill) { > + rem = min(dst_iter.length, actx->fill); This comparison generates a sparse warning due to conflicting types, please fix this and resubmit. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 6/24/21 2:56 AM, Herbert Xu wrote: > On Fri, Jun 18, 2021 at 05:14:11PM -0400, Sean Anderson wrote: >> This uses the sg_miter_*() functions to copy data, instead of doing it >> ourselves. Using sg_copy_buffer() would be better, but this way we don't >> have to keep traversing the beginning of the scatterlist every time we >> do another copy. >> >> In addition to reducing code size, this fixes the following oops >> resulting from failing to kmap the page: > > Thanks for the patch. Just a minor nit: > >> @@ -365,25 +364,13 @@ static int mxs_dcp_aes_block_crypt(struct crypto_async_request *arq) >> >> out_tmp = out_buf; >> last_out_len = actx->fill; >> - while (dst && actx->fill) { >> - if (!split) { >> - dst_buf = sg_virt(dst); >> - dst_off = 0; >> - } >> - rem = min(sg_dma_len(dst) - dst_off, >> - actx->fill); >> - >> - memcpy(dst_buf + dst_off, out_tmp, rem); >> + >> + while (sg_miter_next(&dst_iter) && actx->fill) { >> + rem = min(dst_iter.length, actx->fill); > > This comparison generates a sparse warning due to conflicting types, > please fix this and resubmit. What exactly is the warning here? dst_iter.length is a size_t, and actx->fill is a u32. So fill will be converted to a size_t before the comparison, which is lossless. --Sean
On Thu, Jun 24, 2021 at 10:58:48AM -0400, Sean Anderson wrote: > > What exactly is the warning here? dst_iter.length is a size_t, and > actx->fill is a u32. So fill will be converted to a size_t before the > comparison, which is lossless. It's just the way min works. If you want to shut it up, you can either use a cast or min_t. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 6/24/21 8:16 PM, Herbert Xu wrote: > On Thu, Jun 24, 2021 at 10:58:48AM -0400, Sean Anderson wrote: >> >> What exactly is the warning here? dst_iter.length is a size_t, and >> actx->fill is a u32. So fill will be converted to a size_t before the >> comparison, which is lossless. > > It's just the way min works. If you want to shut it up, you can > either use a cast or min_t. What version of sparse are you using? With sparse 0.6.2, gcc 9.3.0, and with C=1 and W=2 I don't see this warning. --Sean
On Fri, Jun 25, 2021 at 10:49:08AM -0400, Sean Anderson wrote: > > What version of sparse are you using? With sparse 0.6.2, gcc 9.3.0, and > with C=1 and W=2 I don't see this warning. Oh it could be my sparse being out-of-date, I'll get it another go. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, Jun 25, 2021 at 10:49:08AM -0400, Sean Anderson wrote: > > What version of sparse are you using? With sparse 0.6.2, gcc 9.3.0, and > with C=1 and W=2 I don't see this warning. OK I've upgraded my sparse to the latest git tree, but it still gives the same warning, because the two types are of different sizes: $ make C=1 W=1 O=build-compile drivers/crypto/ make[1]: Entering directory '/home/herbert/src/build/kernel/test/build-compile' GEN Makefile CALL ../scripts/checksyscalls.sh CALL ../scripts/atomic/check-atomics.sh CC [M] drivers/crypto/mxs-dcp.o In file included from ../include/linux/kernel.h:15, from ../arch/x86/include/asm/percpu.h:27, from ../arch/x86/include/asm/current.h:6, from ../include/linux/sched.h:12, from ../include/linux/ratelimit.h:6, from ../include/linux/dev_printk.h:16, from ../include/linux/device.h:15, from ../include/linux/dma-mapping.h:7, from ../drivers/crypto/mxs-dcp.c:8: ../drivers/crypto/mxs-dcp.c: In function \u2018mxs_dcp_aes_block_crypt\u2019: ../include/linux/minmax.h:18:28: warning: comparison of distinct pointer types lacks a cast (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) ^~ ../include/linux/minmax.h:32:4: note: in expansion of macro \u2018__typecheck\u2019 (__typecheck(x, y) && __no_side_effects(x, y)) ^~~~~~~~~~~ ../include/linux/minmax.h:42:24: note: in expansion of macro \u2018__safe_cmp\u2019 __builtin_choose_expr(__safe_cmp(x, y), \ ^~~~~~~~~~ ../include/linux/minmax.h:51:19: note: in expansion of macro \u2018__careful_cmp\u2019 #define min(x, y) __careful_cmp(x, y, <) ^~~~~~~~~~~~~ ../drivers/crypto/mxs-dcp.c:369:12: note: in expansion of macro \u2018min\u2019 rem = min(dst_iter.length, actx->fill); ^~~ CHECK ../drivers/crypto/mxs-dcp.c ../drivers/crypto/mxs-dcp.c:369:47: error: incompatible types in comparison expression (different type sizes): ../drivers/crypto/mxs-dcp.c:369:47: unsigned long * ../drivers/crypto/mxs-dcp.c:369:47: unsigned int * make[1]: Leaving directory '/home/herbert/src/build/kernel/test/build-compile' $ In fact as you can see that gcc is warning too. Perhaps you're building on 32-bit? Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 6/27/21 11:25 PM, Herbert Xu wrote: > On Fri, Jun 25, 2021 at 10:49:08AM -0400, Sean Anderson wrote: >> >> What version of sparse are you using? With sparse 0.6.2, gcc 9.3.0, and >> with C=1 and W=2 I don't see this warning. > > OK I've upgraded my sparse to the latest git tree, but it still > gives the same warning, because the two types are of different > sizes: > > $ make C=1 W=1 O=build-compile drivers/crypto/ > make[1]: Entering directory '/home/herbert/src/build/kernel/test/build-compile' > GEN Makefile > CALL ../scripts/checksyscalls.sh > CALL ../scripts/atomic/check-atomics.sh > CC [M] drivers/crypto/mxs-dcp.o > In file included from ../include/linux/kernel.h:15, > from ../arch/x86/include/asm/percpu.h:27, > from ../arch/x86/include/asm/current.h:6, > from ../include/linux/sched.h:12, > from ../include/linux/ratelimit.h:6, > from ../include/linux/dev_printk.h:16, > from ../include/linux/device.h:15, > from ../include/linux/dma-mapping.h:7, > from ../drivers/crypto/mxs-dcp.c:8: > ../drivers/crypto/mxs-dcp.c: In function \u2018mxs_dcp_aes_block_crypt\u2019: > ../include/linux/minmax.h:18:28: warning: comparison of distinct pointer types lacks a cast > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) > ^~ > ../include/linux/minmax.h:32:4: note: in expansion of macro \u2018__typecheck\u2019 > (__typecheck(x, y) && __no_side_effects(x, y)) > ^~~~~~~~~~~ > ../include/linux/minmax.h:42:24: note: in expansion of macro \u2018__safe_cmp\u2019 > __builtin_choose_expr(__safe_cmp(x, y), \ > ^~~~~~~~~~ > ../include/linux/minmax.h:51:19: note: in expansion of macro \u2018__careful_cmp\u2019 > #define min(x, y) __careful_cmp(x, y, <) > ^~~~~~~~~~~~~ > ../drivers/crypto/mxs-dcp.c:369:12: note: in expansion of macro \u2018min\u2019 > rem = min(dst_iter.length, actx->fill); > ^~~ > CHECK ../drivers/crypto/mxs-dcp.c > ../drivers/crypto/mxs-dcp.c:369:47: error: incompatible types in comparison expression (different type sizes): > ../drivers/crypto/mxs-dcp.c:369:47: unsigned long * > ../drivers/crypto/mxs-dcp.c:369:47: unsigned int * > make[1]: Leaving directory '/home/herbert/src/build/kernel/test/build-compile' > $ > > In fact as you can see that gcc is warning too. Perhaps you're > building on 32-bit? Ah, that would be it. Although this module depends on ARCH_MXS || ARCH_MXC and does not (yet) have COMPILE_TEST as an option, so I wonder how you ran into this :) Either way, I will send a v2 with this fixed. > > Thanks, >
diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c index d6a7784d2988..f397cc5bf102 100644 --- a/drivers/crypto/mxs-dcp.c +++ b/drivers/crypto/mxs-dcp.c @@ -170,15 +170,19 @@ static struct dcp *global_sdcp; static int mxs_dcp_start_dma(struct dcp_async_ctx *actx) { + int dma_err; struct dcp *sdcp = global_sdcp; const int chan = actx->chan; uint32_t stat; unsigned long ret; struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan]; - dma_addr_t desc_phys = dma_map_single(sdcp->dev, desc, sizeof(*desc), DMA_TO_DEVICE); + dma_err = dma_mapping_error(sdcp->dev, desc_phys); + if (dma_err) + return dma_err; + reinit_completion(&sdcp->completion[chan]); /* Clear status register. */ @@ -216,18 +220,29 @@ static int mxs_dcp_start_dma(struct dcp_async_ctx *actx) static int mxs_dcp_run_aes(struct dcp_async_ctx *actx, struct skcipher_request *req, int init) { + dma_addr_t key_phys, src_phys, dst_phys; struct dcp *sdcp = global_sdcp; struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan]; struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req); int ret; - dma_addr_t key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key, - 2 * AES_KEYSIZE_128, - DMA_TO_DEVICE); - dma_addr_t src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf, - DCP_BUF_SZ, DMA_TO_DEVICE); - dma_addr_t dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf, - DCP_BUF_SZ, DMA_FROM_DEVICE); + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key, + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE); + ret = dma_mapping_error(sdcp->dev, key_phys); + if (ret) + return ret; + + src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf, + DCP_BUF_SZ, DMA_TO_DEVICE); + ret = dma_mapping_error(sdcp->dev, src_phys); + if (ret) + goto err_src; + + dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf, + DCP_BUF_SZ, DMA_FROM_DEVICE); + ret = dma_mapping_error(sdcp->dev, dst_phys); + if (ret) + goto err_dst; if (actx->fill % AES_BLOCK_SIZE) { dev_err(sdcp->dev, "Invalid block size!\n"); @@ -265,10 +280,12 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx, ret = mxs_dcp_start_dma(actx); aes_done_run: + dma_unmap_single(sdcp->dev, dst_phys, DCP_BUF_SZ, DMA_FROM_DEVICE); +err_dst: + dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE); +err_src: dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128, DMA_TO_DEVICE); - dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE); - dma_unmap_single(sdcp->dev, dst_phys, DCP_BUF_SZ, DMA_FROM_DEVICE); return ret; } @@ -557,6 +574,10 @@ static int mxs_dcp_run_sha(struct ahash_request *req) dma_addr_t buf_phys = dma_map_single(sdcp->dev, sdcp->coh->sha_in_buf, DCP_BUF_SZ, DMA_TO_DEVICE); + ret = dma_mapping_error(sdcp->dev, buf_phys); + if (ret) + return ret; + /* Fill in the DMA descriptor. */ desc->control0 = MXS_DCP_CONTROL0_DECR_SEMAPHORE | MXS_DCP_CONTROL0_INTERRUPT | @@ -589,6 +610,10 @@ static int mxs_dcp_run_sha(struct ahash_request *req) if (rctx->fini) { digest_phys = dma_map_single(sdcp->dev, sdcp->coh->sha_out_buf, DCP_SHA_PAY_SZ, DMA_FROM_DEVICE); + ret = dma_mapping_error(sdcp->dev, digest_phys); + if (ret) + goto done_run; + desc->control0 |= MXS_DCP_CONTROL0_HASH_TERM; desc->payload = digest_phys; }
After calling dma_map_single(), we must also call dma_mapping_error(). This fixes the following warning when compiling with CONFIG_DMA_API_DEBUG: [ 311.241478] WARNING: CPU: 0 PID: 428 at kernel/dma/debug.c:1027 check_unmap+0x79c/0x96c [ 311.249547] DMA-API: mxs-dcp 2280000.crypto: device driver failed to check map error[device address=0x00000000860cb080] [size=32 bytes] [mapped as single] Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/crypto/mxs-dcp.c | 45 +++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 10 deletions(-)