On Mon, 6 Aug 2018 23:12:44 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> Hello Jonathan Cameron,
>
> The patch 915e4e8413da: "crypto: hisilicon - SEC security accelerator
> driver" from Jul 23, 2018, leads to the following static checker
> warning:
>
> drivers/crypto/hisilicon/sec/sec_algs.c:865 sec_alg_skcipher_crypto()
> error: double free of 'split_sizes'
>
> drivers/crypto/hisilicon/sec/sec_algs.c
> 808
> 809 /* Cleanup - all elements in pointer arrays have been coppied */
> 810 kfree(splits_in_nents);
> 811 kfree(splits_in);
> 812 kfree(splits_out_nents);
> 813 kfree(splits_out);
> 814 kfree(split_sizes);
Thanks Dan,
I clearly missed up the error paths when adding the backlog support.
Sorry for the delay, this came in just as I went on vacation. Should get
a patch out later this week.
Easiest is probably going to be just doing this kfree block later, after the last error
handling path.
Jonathan
> ^^^^^^^^^^^
> Free
>
> 815
> 816 /* Grab a big lock for a long time to avoid concurrency issues */
> 817 mutex_lock(&queue->queuelock);
> 818
> 819 /*
> 820 * Can go on to queue if we have space in either:
> 821 * 1) The hardware queue and no software queue
> 822 * 2) The software queue
> 823 * AND there is nothing in the backlog. If there is backlog we
> 824 * have to only queue to the backlog queue and return busy.
> 825 */
> 826 if ((!sec_queue_can_enqueue(queue, steps) &&
> 827 (!queue->havesoftqueue ||
> 828 kfifo_avail(&queue->softqueue) > steps)) ||
> 829 !list_empty(&ctx->backlog)) {
> 830 if ((skreq->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
> 831 list_add_tail(&sec_req->backlog_head, &ctx->backlog);
> 832 mutex_unlock(&queue->queuelock);
> 833 return -EBUSY;
> 834 }
> 835
> 836 ret = -EBUSY;
> 837 mutex_unlock(&queue->queuelock);
> 838 goto err_free_elements;
> ^^^^^^^^^^^^^^^^^^^^^^
> 839 }
> 840 ret = sec_send_request(sec_req, queue);
> 841 mutex_unlock(&queue->queuelock);
> 842 if (ret)
> 843 goto err_free_elements;
> ^^^^^^^^^^^^^^^^^^^^^^
> 844
> 845 return -EINPROGRESS;
> 846
> 847 err_free_elements:
> 848 list_for_each_entry_safe(el, temp, &sec_req->elements, head) {
> 849 list_del(&el->head);
> 850 sec_alg_free_el(el, info);
> 851 }
> 852 if (crypto_skcipher_ivsize(atfm))
> 853 dma_unmap_single(info->dev, sec_req->dma_iv,
> 854 crypto_skcipher_ivsize(atfm),
> 855 DMA_BIDIRECTIONAL);
> 856 err_unmap_out_sg:
> 857 if (skreq->src != skreq->dst)
> 858 sec_unmap_sg_on_err(skreq->dst, steps, splits_out,
> 859 splits_out_nents, sec_req->len_out,
> 860 info->dev);
> 861 err_unmap_in_sg:
> 862 sec_unmap_sg_on_err(skreq->src, steps, splits_in, splits_in_nents,
> 863 sec_req->len_in, info->dev);
> 864 err_free_split_sizes:
> 865 kfree(split_sizes);
> ^^^^^^^^^^^^^^^^^^^
> Double free.
>
> 866
> 867 return ret;
> 868 }
>
> regards,
> dan carpenter
