2020-11-10 10:53:04

by Dan Carpenter

[permalink] [raw]
Subject: drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c:412 sun8i_ce_hash_run() warn: possible memory leak of 'result'

tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 407ab579637ced6dc32cfb2295afb7259cca4b22
commit: 56f6d5aee88d129b2424902cd630f10794550763 crypto: sun8i-ce - support hash algorithms
config: x86_64-randconfig-m001-20201109 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>

smatch warnings:
drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c:412 sun8i_ce_hash_run() warn: possible memory leak of 'result'

vim +/result +412 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c

56f6d5aee88d129 Corentin Labbe 2020-09-18 249 int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
56f6d5aee88d129 Corentin Labbe 2020-09-18 250 {
56f6d5aee88d129 Corentin Labbe 2020-09-18 251 struct ahash_request *areq = container_of(breq, struct ahash_request, base);
56f6d5aee88d129 Corentin Labbe 2020-09-18 252 struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
56f6d5aee88d129 Corentin Labbe 2020-09-18 253 struct ahash_alg *alg = __crypto_ahash_alg(tfm->base.__crt_alg);
56f6d5aee88d129 Corentin Labbe 2020-09-18 254 struct sun8i_ce_hash_reqctx *rctx = ahash_request_ctx(areq);
56f6d5aee88d129 Corentin Labbe 2020-09-18 255 struct sun8i_ce_alg_template *algt;
56f6d5aee88d129 Corentin Labbe 2020-09-18 256 struct sun8i_ce_dev *ce;
56f6d5aee88d129 Corentin Labbe 2020-09-18 257 struct sun8i_ce_flow *chan;
56f6d5aee88d129 Corentin Labbe 2020-09-18 258 struct ce_task *cet;
56f6d5aee88d129 Corentin Labbe 2020-09-18 259 struct scatterlist *sg;
56f6d5aee88d129 Corentin Labbe 2020-09-18 260 int nr_sgs, flow, err;
56f6d5aee88d129 Corentin Labbe 2020-09-18 261 unsigned int len;
56f6d5aee88d129 Corentin Labbe 2020-09-18 262 u32 common;
56f6d5aee88d129 Corentin Labbe 2020-09-18 263 u64 byte_count;
56f6d5aee88d129 Corentin Labbe 2020-09-18 264 __le32 *bf;
56f6d5aee88d129 Corentin Labbe 2020-09-18 265 void *buf;
56f6d5aee88d129 Corentin Labbe 2020-09-18 266 int j, i, todo;
56f6d5aee88d129 Corentin Labbe 2020-09-18 267 int nbw = 0;
56f6d5aee88d129 Corentin Labbe 2020-09-18 268 u64 fill, min_fill;
56f6d5aee88d129 Corentin Labbe 2020-09-18 269 __be64 *bebits;
56f6d5aee88d129 Corentin Labbe 2020-09-18 270 __le64 *lebits;
56f6d5aee88d129 Corentin Labbe 2020-09-18 271 void *result;
56f6d5aee88d129 Corentin Labbe 2020-09-18 272 u64 bs;
56f6d5aee88d129 Corentin Labbe 2020-09-18 273 int digestsize;
56f6d5aee88d129 Corentin Labbe 2020-09-18 274 dma_addr_t addr_res, addr_pad;
56f6d5aee88d129 Corentin Labbe 2020-09-18 275
56f6d5aee88d129 Corentin Labbe 2020-09-18 276 algt = container_of(alg, struct sun8i_ce_alg_template, alg.hash);
56f6d5aee88d129 Corentin Labbe 2020-09-18 277 ce = algt->ce;
56f6d5aee88d129 Corentin Labbe 2020-09-18 278
56f6d5aee88d129 Corentin Labbe 2020-09-18 279 bs = algt->alg.hash.halg.base.cra_blocksize;
56f6d5aee88d129 Corentin Labbe 2020-09-18 280 digestsize = algt->alg.hash.halg.digestsize;
56f6d5aee88d129 Corentin Labbe 2020-09-18 281 if (digestsize == SHA224_DIGEST_SIZE)
56f6d5aee88d129 Corentin Labbe 2020-09-18 282 digestsize = SHA256_DIGEST_SIZE;
56f6d5aee88d129 Corentin Labbe 2020-09-18 283 if (digestsize == SHA384_DIGEST_SIZE)
56f6d5aee88d129 Corentin Labbe 2020-09-18 284 digestsize = SHA512_DIGEST_SIZE;
56f6d5aee88d129 Corentin Labbe 2020-09-18 285
56f6d5aee88d129 Corentin Labbe 2020-09-18 286 /* the padding could be up to two block. */
56f6d5aee88d129 Corentin Labbe 2020-09-18 287 buf = kzalloc(bs * 2, GFP_KERNEL | GFP_DMA);
^^^^^^^^^^^^^
"buf" is leaked as well.

56f6d5aee88d129 Corentin Labbe 2020-09-18 288 if (!buf)
56f6d5aee88d129 Corentin Labbe 2020-09-18 289 return -ENOMEM;
56f6d5aee88d129 Corentin Labbe 2020-09-18 290 bf = (__le32 *)buf;
56f6d5aee88d129 Corentin Labbe 2020-09-18 291
56f6d5aee88d129 Corentin Labbe 2020-09-18 292 result = kzalloc(digestsize, GFP_KERNEL | GFP_DMA);
^^^^^^^^^^^^^^^^^


56f6d5aee88d129 Corentin Labbe 2020-09-18 293 if (!result)
56f6d5aee88d129 Corentin Labbe 2020-09-18 294 return -ENOMEM;
56f6d5aee88d129 Corentin Labbe 2020-09-18 295
56f6d5aee88d129 Corentin Labbe 2020-09-18 296 flow = rctx->flow;
56f6d5aee88d129 Corentin Labbe 2020-09-18 297 chan = &ce->chanlist[flow];
56f6d5aee88d129 Corentin Labbe 2020-09-18 298
56f6d5aee88d129 Corentin Labbe 2020-09-18 299 #ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
56f6d5aee88d129 Corentin Labbe 2020-09-18 300 algt->stat_req++;
56f6d5aee88d129 Corentin Labbe 2020-09-18 301 #endif
56f6d5aee88d129 Corentin Labbe 2020-09-18 302 dev_dbg(ce->dev, "%s %s len=%d\n", __func__, crypto_tfm_alg_name(areq->base.tfm), areq->nbytes);
56f6d5aee88d129 Corentin Labbe 2020-09-18 303
56f6d5aee88d129 Corentin Labbe 2020-09-18 304 cet = chan->tl;
56f6d5aee88d129 Corentin Labbe 2020-09-18 305 memset(cet, 0, sizeof(struct ce_task));
56f6d5aee88d129 Corentin Labbe 2020-09-18 306
56f6d5aee88d129 Corentin Labbe 2020-09-18 307 cet->t_id = cpu_to_le32(flow);
56f6d5aee88d129 Corentin Labbe 2020-09-18 308 common = ce->variant->alg_hash[algt->ce_algo_id];
56f6d5aee88d129 Corentin Labbe 2020-09-18 309 common |= CE_COMM_INT;
56f6d5aee88d129 Corentin Labbe 2020-09-18 310 cet->t_common_ctl = cpu_to_le32(common);
56f6d5aee88d129 Corentin Labbe 2020-09-18 311
56f6d5aee88d129 Corentin Labbe 2020-09-18 312 cet->t_sym_ctl = 0;
56f6d5aee88d129 Corentin Labbe 2020-09-18 313 cet->t_asym_ctl = 0;
56f6d5aee88d129 Corentin Labbe 2020-09-18 314
56f6d5aee88d129 Corentin Labbe 2020-09-18 315 nr_sgs = dma_map_sg(ce->dev, areq->src, sg_nents(areq->src), DMA_TO_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 316 if (nr_sgs <= 0 || nr_sgs > MAX_SG) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 317 dev_err(ce->dev, "Invalid sg number %d\n", nr_sgs);
56f6d5aee88d129 Corentin Labbe 2020-09-18 318 err = -EINVAL;
56f6d5aee88d129 Corentin Labbe 2020-09-18 319 goto theend;

Leaked on some of these gotos.

56f6d5aee88d129 Corentin Labbe 2020-09-18 320 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 321
56f6d5aee88d129 Corentin Labbe 2020-09-18 322 len = areq->nbytes;
56f6d5aee88d129 Corentin Labbe 2020-09-18 323 for_each_sg(areq->src, sg, nr_sgs, i) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 324 cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
56f6d5aee88d129 Corentin Labbe 2020-09-18 325 todo = min(len, sg_dma_len(sg));
56f6d5aee88d129 Corentin Labbe 2020-09-18 326 cet->t_src[i].len = cpu_to_le32(todo / 4);
56f6d5aee88d129 Corentin Labbe 2020-09-18 327 len -= todo;
56f6d5aee88d129 Corentin Labbe 2020-09-18 328 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 329 if (len > 0) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 330 dev_err(ce->dev, "remaining len %d\n", len);
56f6d5aee88d129 Corentin Labbe 2020-09-18 331 err = -EINVAL;
56f6d5aee88d129 Corentin Labbe 2020-09-18 332 goto theend;
56f6d5aee88d129 Corentin Labbe 2020-09-18 333 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 334 addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 335 cet->t_dst[0].addr = cpu_to_le32(addr_res);
56f6d5aee88d129 Corentin Labbe 2020-09-18 336 cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
56f6d5aee88d129 Corentin Labbe 2020-09-18 337 if (dma_mapping_error(ce->dev, addr_res)) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 338 dev_err(ce->dev, "DMA map dest\n");
56f6d5aee88d129 Corentin Labbe 2020-09-18 339 err = -EINVAL;
56f6d5aee88d129 Corentin Labbe 2020-09-18 340 goto theend;
56f6d5aee88d129 Corentin Labbe 2020-09-18 341 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 342
56f6d5aee88d129 Corentin Labbe 2020-09-18 343 byte_count = areq->nbytes;
56f6d5aee88d129 Corentin Labbe 2020-09-18 344 j = 0;
56f6d5aee88d129 Corentin Labbe 2020-09-18 345 bf[j++] = cpu_to_le32(0x80);
56f6d5aee88d129 Corentin Labbe 2020-09-18 346
56f6d5aee88d129 Corentin Labbe 2020-09-18 347 if (bs == 64) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 348 fill = 64 - (byte_count % 64);
56f6d5aee88d129 Corentin Labbe 2020-09-18 349 min_fill = 2 * sizeof(u32) + (nbw ? 0 : sizeof(u32));
56f6d5aee88d129 Corentin Labbe 2020-09-18 350 } else {
56f6d5aee88d129 Corentin Labbe 2020-09-18 351 fill = 128 - (byte_count % 128);
56f6d5aee88d129 Corentin Labbe 2020-09-18 352 min_fill = 4 * sizeof(u32) + (nbw ? 0 : sizeof(u32));
56f6d5aee88d129 Corentin Labbe 2020-09-18 353 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 354
56f6d5aee88d129 Corentin Labbe 2020-09-18 355 if (fill < min_fill)
56f6d5aee88d129 Corentin Labbe 2020-09-18 356 fill += bs;
56f6d5aee88d129 Corentin Labbe 2020-09-18 357
56f6d5aee88d129 Corentin Labbe 2020-09-18 358 j += (fill - min_fill) / sizeof(u32);
56f6d5aee88d129 Corentin Labbe 2020-09-18 359
56f6d5aee88d129 Corentin Labbe 2020-09-18 360 switch (algt->ce_algo_id) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 361 case CE_ID_HASH_MD5:
56f6d5aee88d129 Corentin Labbe 2020-09-18 362 lebits = (__le64 *)&bf[j];
56f6d5aee88d129 Corentin Labbe 2020-09-18 363 *lebits = cpu_to_le64(byte_count << 3);
56f6d5aee88d129 Corentin Labbe 2020-09-18 364 j += 2;
56f6d5aee88d129 Corentin Labbe 2020-09-18 365 break;
56f6d5aee88d129 Corentin Labbe 2020-09-18 366 case CE_ID_HASH_SHA1:
56f6d5aee88d129 Corentin Labbe 2020-09-18 367 case CE_ID_HASH_SHA224:
56f6d5aee88d129 Corentin Labbe 2020-09-18 368 case CE_ID_HASH_SHA256:
56f6d5aee88d129 Corentin Labbe 2020-09-18 369 bebits = (__be64 *)&bf[j];
56f6d5aee88d129 Corentin Labbe 2020-09-18 370 *bebits = cpu_to_be64(byte_count << 3);
56f6d5aee88d129 Corentin Labbe 2020-09-18 371 j += 2;
56f6d5aee88d129 Corentin Labbe 2020-09-18 372 break;
56f6d5aee88d129 Corentin Labbe 2020-09-18 373 case CE_ID_HASH_SHA384:
56f6d5aee88d129 Corentin Labbe 2020-09-18 374 case CE_ID_HASH_SHA512:
56f6d5aee88d129 Corentin Labbe 2020-09-18 375 bebits = (__be64 *)&bf[j];
56f6d5aee88d129 Corentin Labbe 2020-09-18 376 *bebits = cpu_to_be64(byte_count >> 61);
56f6d5aee88d129 Corentin Labbe 2020-09-18 377 j += 2;
56f6d5aee88d129 Corentin Labbe 2020-09-18 378 bebits = (__be64 *)&bf[j];
56f6d5aee88d129 Corentin Labbe 2020-09-18 379 *bebits = cpu_to_be64(byte_count << 3);
56f6d5aee88d129 Corentin Labbe 2020-09-18 380 j += 2;
56f6d5aee88d129 Corentin Labbe 2020-09-18 381 break;
56f6d5aee88d129 Corentin Labbe 2020-09-18 382 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 383
56f6d5aee88d129 Corentin Labbe 2020-09-18 384 addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 385 cet->t_src[i].addr = cpu_to_le32(addr_pad);
56f6d5aee88d129 Corentin Labbe 2020-09-18 386 cet->t_src[i].len = cpu_to_le32(j);
56f6d5aee88d129 Corentin Labbe 2020-09-18 387 if (dma_mapping_error(ce->dev, addr_pad)) {
56f6d5aee88d129 Corentin Labbe 2020-09-18 388 dev_err(ce->dev, "DMA error on padding SG\n");
56f6d5aee88d129 Corentin Labbe 2020-09-18 389 err = -EINVAL;
56f6d5aee88d129 Corentin Labbe 2020-09-18 390 goto theend;
56f6d5aee88d129 Corentin Labbe 2020-09-18 391 }
56f6d5aee88d129 Corentin Labbe 2020-09-18 392
56f6d5aee88d129 Corentin Labbe 2020-09-18 393 if (ce->variant->hash_t_dlen_in_bits)
56f6d5aee88d129 Corentin Labbe 2020-09-18 394 cet->t_dlen = cpu_to_le32((areq->nbytes + j * 4) * 8);
56f6d5aee88d129 Corentin Labbe 2020-09-18 395 else
56f6d5aee88d129 Corentin Labbe 2020-09-18 396 cet->t_dlen = cpu_to_le32(areq->nbytes / 4 + j);
56f6d5aee88d129 Corentin Labbe 2020-09-18 397
56f6d5aee88d129 Corentin Labbe 2020-09-18 398 chan->timeout = areq->nbytes;
56f6d5aee88d129 Corentin Labbe 2020-09-18 399
56f6d5aee88d129 Corentin Labbe 2020-09-18 400 err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(areq->base.tfm));
56f6d5aee88d129 Corentin Labbe 2020-09-18 401
56f6d5aee88d129 Corentin Labbe 2020-09-18 402 dma_unmap_single(ce->dev, addr_pad, j * 4, DMA_TO_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 403 dma_unmap_sg(ce->dev, areq->src, nr_sgs, DMA_TO_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 404 dma_unmap_single(ce->dev, addr_res, digestsize, DMA_FROM_DEVICE);
56f6d5aee88d129 Corentin Labbe 2020-09-18 405
56f6d5aee88d129 Corentin Labbe 2020-09-18 406 kfree(buf);
56f6d5aee88d129 Corentin Labbe 2020-09-18 407
56f6d5aee88d129 Corentin Labbe 2020-09-18 408 memcpy(areq->result, result, algt->alg.hash.halg.digestsize);
56f6d5aee88d129 Corentin Labbe 2020-09-18 409 kfree(result);
56f6d5aee88d129 Corentin Labbe 2020-09-18 410 theend:
56f6d5aee88d129 Corentin Labbe 2020-09-18 411 crypto_finalize_hash_request(engine, breq, err);
56f6d5aee88d129 Corentin Labbe 2020-09-18 @412 return 0;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (13.08 kB)
.config.gz (46.14 kB)
Download all attachments

2020-11-11 08:04:12

by Corentin LABBE

[permalink] [raw]
Subject: Re: drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c:412 sun8i_ce_hash_run() warn: possible memory leak of 'result'

On Tue, Nov 10, 2020 at 01:47:37PM +0300, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 407ab579637ced6dc32cfb2295afb7259cca4b22
> commit: 56f6d5aee88d129b2424902cd630f10794550763 crypto: sun8i-ce - support hash algorithms
> config: x86_64-randconfig-m001-20201109 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>

Hello

Thanks for the report, I will send a patch soon.
Note that you should send this report to the maintainer also (but this time it is me with another address, so its fine).

Regards

2020-11-11 10:01:18

by Dan Carpenter

[permalink] [raw]
Subject: Re: drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c:412 sun8i_ce_hash_run() warn: possible memory leak of 'result'

On Wed, Nov 11, 2020 at 09:01:34AM +0100, LABBE Corentin wrote:
> On Tue, Nov 10, 2020 at 01:47:37PM +0300, Dan Carpenter wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 407ab579637ced6dc32cfb2295afb7259cca4b22
> > commit: 56f6d5aee88d129b2424902cd630f10794550763 crypto: sun8i-ce - support hash algorithms
> > config: x86_64-randconfig-m001-20201109 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: Dan Carpenter <[email protected]>
> >
>
> Hello
>
> Thanks for the report, I will send a patch soon.
> Note that you should send this report to the maintainer also (but this time it is me with another address, so its fine).
>

These are automated emails from the kbuild bot. I just look over the
Smatch warnings and forward them. It don't know how the CC list is
chosen. I guess just from who signed off on the patch... My guess is
that people would be annoyed if we CC'd more people.

Generally, we have a really good success rate with people fixing these
warnings. I recently had a case where the zero day bot email wasn't
clear, but I caught that because I'm looking at new Smatch warnings on
my own system. There was another case, where the bug was fixed and then
re-introduced via a bad merge and we almost missed that because it was
marked as old. Fortunately, I discoverd it while looking at a different
bug.

regards,
dan carpenter