[bug report] crypto: user - Implement a generic crypto statistics
Hello Corentin Labbe,
The patch cac5818c25d0: "crypto: user - Implement a generic crypto
statistics" from Sep 19, 2018, leads to the following static checker
warning:
crypto/crypto_user_stat.c:53 crypto_report_aead()
warn: check that 'raead' doesn't leak information (struct has holes)
crypto/crypto_user_stat.c
34 static int crypto_report_aead(struct sk_buff *skb, struct crypto_alg
*alg)
35 {
36 struct crypto_stat raead;
^^^
37 u64 v64;
38 u32 v32;
39
40 strncpy(raead.type, "aead", sizeof(raead.type));
41
42 v32 = atomic_read(>encrypt_cnt);
43 raead.stat_encrypt_cnt = v32;
44 v64 = atomic64_read(>encrypt_tlen);
45 raead.stat_encrypt_tlen = v64;
46 v32 = atomic_read(>decrypt_cnt);
47 raead.stat_decrypt_cnt = v32;
48 v64 = atomic64_read(>decrypt_tlen);
49 raead.stat_decrypt_tlen = v64;
50 v32 = atomic_read(>aead_err_cnt);
51 raead.stat_aead_err_cnt = v32;
52
53 if (nla_put(skb, CRYPTOCFGA_STAT_AEAD,
54 sizeof(struct crypto_stat), ))
^^
We haven't totally initialized raead and also it apparently has struct
holes after the u64s.
55 goto nla_put_failure;
56 return 0;
57
58 nla_put_failure:
59 return -EMSGSIZE;
60 }
See also:
crypto/crypto_user_stat.c:53 crypto_report_aead() warn: check that 'raead'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:81 crypto_report_cipher() warn: check that 'rcipher'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:108 crypto_report_comp() warn: check that 'rcomp'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:135 crypto_report_acomp() warn: check that 'racomp'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:166 crypto_report_akcipher() warn: check that
'rakcipher' doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:191 crypto_report_kpp() warn: check that 'rkpp'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:215 crypto_report_ahash() warn: check that 'rhash'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:239 crypto_report_shash() warn: check that 'rhash'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:265 crypto_report_rng() warn: check that 'rrng'
doesn't leak information (struct has holes)
crypto/crypto_user_stat.c:295 crypto_reportstat_one() warn: check that 'rl'
doesn't leak information (struct has holes)
regards,
dan carpenter
[bug report] crypto: chtls - Inline TLS record Tx
Hello Atul Gupta,
The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from
Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_io.c:1036 chtls_sendmsg()
warn: assigning signed to unsigned: 'csk->tlshws.txleft = recordsz'
'(-14),0-u16max'
drivers/crypto/chelsio/chtls/chtls_io.c
1018
1019 while (msg_data_left(msg)) {
1020 int copy = 0;
1021
1022 skb = skb_peek_tail(>txq);
1023 if (skb) {
1024 copy = mss - skb->len;
1025 skb->ip_summed = CHECKSUM_UNNECESSARY;
1026 }
1027 if (!csk_mem_free(cdev, sk))
1028 goto wait_for_sndbuf;
1029
1030 if (is_tls_tx(csk) && !csk->tlshws.txleft) {
1031 struct tls_hdr hdr;
1032
1033 recordsz = tls_header_read(,
>msg_iter);
^
What if "recordsz" is -EFAULT?
1034 size -= TLS_HEADER_LENGTH;
1035 hdrlen += TLS_HEADER_LENGTH;
1036 csk->tlshws.txleft = recordsz;
1037 csk->tlshws.type = hdr.type;
1038 if (skb)
1039 ULP_SKB_CB(skb)->ulp.tls.type =
hdr.type;
1040 }
1041
1042 if (!skb || (ULP_SKB_CB(skb)->flags &
ULPCB_FLAG_NO_APPEND) ||
1043 copy <= 0) {
1044 new_buf:
1045 if (skb) {
1046 tx_skb_finalize(skb);
1047 push_frames_if_head(sk);
1048 }
regards,
dan carpenter
[bug report] crypto: omap-aes - Add support for GCM mode
Hello Tero Kristo,
This is a semi-automatic email about new static checker warnings.
The patch ad18cc9d0f91: "crypto: omap-aes - Add support for GCM mode"
from May 24, 2017, leads to the following Smatch complaint:
drivers/crypto/omap-aes.c:1262 omap_aes_probe()
error: we previously assumed 'dd->pdata->aead_algs_info' could be null
(see line 1237)
drivers/crypto/omap-aes.c
1236
1237 if (dd->pdata->aead_algs_info &&
^
This check could presumably be removed?
1238 !dd->pdata->aead_algs_info->registered) {
1239 for (i = 0; i < dd->pdata->aead_algs_info->size; i++) {
1240 aalg = >pdata->aead_algs_info->algs_list[i];
1241 algp = >base;
1242
1243 pr_debug("reg alg: %s\n", algp->cra_name);
1244 INIT_LIST_HEAD(>cra_list);
1245
1246 err = crypto_register_aead(aalg);
1247 if (err)
1248 goto err_aead_algs;
1249
1250 dd->pdata->aead_algs_info->registered++;
1251 }
1252 }
1253
1254 err = sysfs_create_group(>kobj, _aes_attr_group);
1255 if (err) {
1256 dev_err(dev, "could not create sysfs device attrs\n");
1257 goto err_aead_algs;
1258 }
1259
1260 return 0;
1261 err_aead_algs:
1262 for (i = dd->pdata->aead_algs_info->registered - 1; i >= 0;
i--) {
^
Otherwise the error handling needs a check as well.
1263 aalg = >pdata->aead_algs_info->algs_list[i];
1264 crypto_unregister_aead(aalg);
regards,
dan carpenter
[bug report] crypto: hisilicon - SEC security accelerator driver
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);
^^^
Free
815
816 /* Grab a big lock for a long time to avoid concurrency issues
*/
817 mutex_lock(>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 ||
828kfifo_avail(>softqueue) > steps)) ||
829 !list_empty(>backlog)) {
830 if ((skreq->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
831 list_add_tail(_req->backlog_head,
>backlog);
832 mutex_unlock(>queuelock);
833 return -EBUSY;
834 }
835
836 ret = -EBUSY;
837 mutex_unlock(>queuelock);
838 goto err_free_elements;
^^
839 }
840 ret = sec_send_request(sec_req, queue);
841 mutex_unlock(>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, _req->elements, head) {
849 list_del(>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
[bug report] crypto: ccp - Add DOWNLOAD_FIRMWARE SEV command
Hello Janakarajan Natarajan,
The patch edd303ff0e9e: "crypto: ccp - Add DOWNLOAD_FIRMWARE SEV
command" from May 25, 2018, leads to the following static checker
warning:
drivers/crypto/ccp/psp-dev.c:397 sev_get_api_version()
error: uninitialized symbol 'error'.
drivers/crypto/ccp/psp-dev.c
389 static int sev_get_api_version(void)
390 {
391 struct sev_user_data_status *status;
392 int error, ret;
393
394 status = _master->status_cmd_buf;
395 ret = sev_platform_status(status, );
396 if (ret) {
397 dev_err(psp_master->dev,
398 "SEV: failed to get status. Error: %#x\n",
error);
sev_platform_status() could be defined as a no-op or there is an error
path where *error isn't set.
399 return 1;
400 }
401
402 psp_master->api_major = status->api_major;
403 psp_master->api_minor = status->api_minor;
404 psp_master->build = status->build;
405
406 return 0;
407 }
regards,
dan carpenter
[bug report] crypto: chtls - Register chtls with net tls
Hello Atul Gupta,
The patch a08943947873: "crypto: chtls - Register chtls with net tls"
from Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_main.c:352 chtls_recv_packet()
error: double free of 'skb'
drivers/crypto/chelsio/chtls/chtls_main.c
337 static int chtls_recv_packet(struct chtls_dev *cdev,
338 const struct pkt_gl *gl, const __be64 *rsp)
339 {
340 unsigned int opcode = *(u8 *)rsp;
341 struct sk_buff *skb;
342 int ret;
343
344 skb = copy_gl_to_skb_pkt(gl, rsp, cdev->lldi->sge_pktshift);
345 if (!skb)
346 return -ENOMEM;
347
348 ret = chtls_handlers[opcode](cdev, skb);
349 if (ret & CPL_RET_BUF_DONE)
350 kfree_skb(skb);
This is a false positive because Smatch doesn't parse the test for
CPL_RET_BUF_DONE set correctly. It's not that complicated for me to fix
that in Smatch so I will eventually. But really this is risky code.
A bunch of these handler functions return -EINVAL. If they return an
odd numbered error code instead then we free skb which is pretty
subtle so far as APIs are concerned.
Looking at it now, I think we probably should be freeing skb on those
paths. The current code looks leaky to me.
351
352 return 0;
353 }
regards,
dan carpenter
Re: [PATCH 2/5] crypto: chtls: wait for memory sendmsg, sendpage
On Mon, May 14, 2018 at 04:30:56PM +0530, Atul Gupta wrote:
> Reported-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
There isn't a commit message for this. It should say what the user
visible effects of this bug are. I haven't seen Gustavo's bug report,
but probably copy and pasting that would be good?
> ---
> drivers/crypto/chelsio/chtls/chtls.h | 1 +
> drivers/crypto/chelsio/chtls/chtls_io.c | 90
> +--
> drivers/crypto/chelsio/chtls/chtls_main.c | 1 +
> 3 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls.h
> b/drivers/crypto/chelsio/chtls/chtls.h
> index f4b8f1e..778c194 100644
> --- a/drivers/crypto/chelsio/chtls/chtls.h
> +++ b/drivers/crypto/chelsio/chtls/chtls.h
> @@ -149,6 +149,7 @@ struct chtls_dev {
> struct list_head rcu_node;
> struct list_head na_node;
> unsigned int send_page_order;
> + int max_host_sndbuf;
> struct key_map kmap;
> };
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 5a75be4..a4c7d2d 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -914,6 +914,78 @@ static u16 tls_header_read(struct tls_hdr *thdr, struct
> iov_iter *from)
> return (__force u16)cpu_to_be16(thdr->length);
> }
>
> +static int csk_mem_free(struct chtls_dev *cdev, struct sock *sk)
> +{
> + return (cdev->max_host_sndbuf - sk->sk_wmem_queued) > 0;
Why not just say:
return (max_host_sndbuf > sk->sk_wmem_queued);
> +}
> +
> +static int csk_wait_memory(struct chtls_dev *cdev,
> +struct sock *sk, long *timeo_p)
> +{
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + int sndbuf, err = 0;
> + long current_timeo;
> + long vm_wait = 0;
> + bool noblock;
> +
> + current_timeo = *timeo_p;
> + noblock = (*timeo_p ? false : true);
> sndbuf = cdev->max_host_sndbuf;
> + if (sndbuf > sk->sk_wmem_queued) {
You could use it here:
if (csk_mem_free(cdev, sk)) {
> + current_timeo = (prandom_u32() % (HZ / 5)) + 2;
> + vm_wait = (prandom_u32() % (HZ / 5)) + 2;
> + }
> +
> + add_wait_queue(sk_sleep(sk), );
> + while (1) {
> + sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> +
> + if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> + goto do_error;
> + if (!*timeo_p) {
> + if (noblock)
> + set_bit(SOCK_NOSPACE, >sk_socket->flags);
> + goto do_nonblock;
There are missing curly braces here. I feel like these gotos make the
code worse. They don't remove duplicate lines of code. They just
spread things out so that you have to jump around to understand this
code. It's like being a kangaroo.
if (noblock) {
set_bit(SOCK_NOSPACE, >sk_socket->flags);
err = -EAGAIN;
goto remove_queue;
}
I always like picking a descriptive label names instead of "out:"
> + }
> + if (signal_pending(current))
> + goto do_interrupted;
> + sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> + if (sndbuf > sk->sk_wmem_queued && !vm_wait)
> + break;
if (csk_mem_free(cdev, sk) && !vm_wait)
> +
> + set_bit(SOCK_NOSPACE, >sk_socket->flags);
> + sk->sk_write_pending++;
> + sk_wait_event(sk, _timeo, sk->sk_err ||
> + (sk->sk_shutdown & SEND_SHUTDOWN) ||
> + (sndbuf > sk->sk_wmem_queued && !vm_wait), );
(csk_mem_free(cdev, sk) && !vm_wait), );
> + sk->sk_write_pending--;
> +
> + if (vm_wait) {
> + vm_wait -= current_timeo;
> + current_timeo = *timeo_p;
> + if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
> + current_timeo -= vm_wait;
> + if (current_timeo < 0)
> + current_timeo = 0;
> + }
> + vm_wait = 0;
> + }
> + *timeo_p = current_timeo;
> + }
> +out:
> + remove_wait_queue(sk_sleep(sk), );
> + return err;
> +do_error:
> + err = -EPIPE;
> + goto out;
> +do_nonblock:
> + err = -EAGAIN;
> + goto out;
> +do_interrupted:
> + err = sock_intr_errno(*timeo_p);
> + goto out;
> +}
> +
regards,
dan carpenter
Re: [PATCH] crypto: chtls - fix a missing-check bug
Hi Wenwen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Wenwen-Wang/crypto-chtls-fix-a-missing-check-bug/20180506-091039
base:
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
:: branch date: 26 hours ago
:: commit date: 26 hours ago
New smatch warnings:
drivers/crypto/chelsio/chtls/chtls_main.c:496 do_chtls_setsockopt() warn:
potential pointer math issue ('crypto_info' is a 32 bit pointer)
Old smatch warnings:
drivers/crypto/chelsio/chtls/chtls_main.c:253 chtls_uld_add() error: buffer
overflow 'cdev->rspq_skb_cache' 32 <= 32
drivers/crypto/chelsio/chtls/chtls_main.c:350 chtls_recv_packet() error: double
free of 'skb'
drivers/crypto/chelsio/chtls/chtls_main.c:390 chtls_recv_rsp() error: double
free of 'skb'
drivers/crypto/chelsio/chtls/chtls_main.c:408 chtls_recv() error: double free
of 'skb'
#
https://github.com/0day-ci/linux/commit/183b5e3e71c75e3149dac2698883f0bd63a89c75
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 183b5e3e71c75e3149dac2698883f0bd63a89c75
vim +496 drivers/crypto/chelsio/chtls/chtls_main.c
a0894394 Atul Gupta 2018-03-31 461
a0894394 Atul Gupta 2018-03-31 462 static int do_chtls_setsockopt(struct
sock *sk, int optname,
a0894394 Atul Gupta 2018-03-31 463 char __user
*optval, unsigned int optlen)
a0894394 Atul Gupta 2018-03-31 464 {
a0894394 Atul Gupta 2018-03-31 465struct tls_crypto_info *crypto_info,
tmp_crypto_info;
a0894394 Atul Gupta 2018-03-31 466struct chtls_sock *csk;
a0894394 Atul Gupta 2018-03-31 467int keylen;
a0894394 Atul Gupta 2018-03-31 468int rc = 0;
a0894394 Atul Gupta 2018-03-31 469
a0894394 Atul Gupta 2018-03-31 470csk = rcu_dereference_sk_user_data(sk);
a0894394 Atul Gupta 2018-03-31 471
a0894394 Atul Gupta 2018-03-31 472if (!optval || optlen <
sizeof(*crypto_info)) {
a0894394 Atul Gupta 2018-03-31 473rc = -EINVAL;
a0894394 Atul Gupta 2018-03-31 474goto out;
a0894394 Atul Gupta 2018-03-31 475}
a0894394 Atul Gupta 2018-03-31 476
a0894394 Atul Gupta 2018-03-31 477rc = copy_from_user(_crypto_info,
optval, sizeof(*crypto_info));
a0894394 Atul Gupta 2018-03-31 478if (rc) {
a0894394 Atul Gupta 2018-03-31 479rc = -EFAULT;
a0894394 Atul Gupta 2018-03-31 480goto out;
a0894394 Atul Gupta 2018-03-31 481}
a0894394 Atul Gupta 2018-03-31 482
a0894394 Atul Gupta 2018-03-31 483/* check version */
a0894394 Atul Gupta 2018-03-31 484if (tmp_crypto_info.version !=
TLS_1_2_VERSION) {
a0894394 Atul Gupta 2018-03-31 485rc = -ENOTSUPP;
a0894394 Atul Gupta 2018-03-31 486goto out;
a0894394 Atul Gupta 2018-03-31 487}
a0894394 Atul Gupta 2018-03-31 488
a0894394 Atul Gupta 2018-03-31 489crypto_info = (struct tls_crypto_info
*)>tlshws.crypto_info;
a0894394 Atul Gupta 2018-03-31 490
a0894394 Atul Gupta 2018-03-31 491switch (tmp_crypto_info.cipher_type) {
a0894394 Atul Gupta 2018-03-31 492case TLS_CIPHER_AES_GCM_128: {
183b5e3e Wenwen Wang 2018-05-05 493/* Obtain version and type from
previous copy */
183b5e3e Wenwen Wang 2018-05-05 494crypto_info[0] =
tmp_crypto_info;
183b5e3e Wenwen Wang 2018-05-05 495/* Now copy the following data
*/
183b5e3e Wenwen Wang 2018-05-05 @496rc = copy_from_user(crypto_info
+ sizeof(*crypto_info),
183b5e3e Wenwen Wang 2018-05-05 497optval +
sizeof(*crypto_info),
183b5e3e Wenwen Wang 2018-05-05 498sizeof(struct
tls12_crypto_info_aes_gcm_128)
183b5e3e Wenwen Wang 2018-05-05 499-
sizeof(*crypto_info));
a0894394 Atul Gupta 2018-03-31 500
a0894394 Atul Gupta 2018-03-31 501if (rc) {
a0894394 Atul Gupta 2018-03-31 502rc = -EFAULT;
a0894394 Atul Gupta 2018-03-31 503goto out;
a0894394 Atul Gupta 2018-03-31 504}
a0894394 Atul Gupta 2018-03-31 505
a0894394 Atul Gupta 2018-03-31 506keylen =
TLS_CIPHER_AES_GCM_128_KEY_SIZE;
a0894394 Atul Gupta 2018-03-31 507rc = chtls_setkey(csk, keylen,
optname);
a0894394 Atul Gupta 2018-03-31 508break;
a0894394 Atul Gupta 2018-03-31 509}
a0894394 Atul Gupta 2018-03-31 510default:
a0894394 Atul Gupta 2018-03-31 511rc = -EINVAL;
a0894394 Atul Gupta 2018-03-31 512goto out;
a0894394 Atul Gupta 2018-03-31 513}
a0894394 Atul Gupta 2018-03-31 514 out:
a0894394 Atul Gupta 2018-03-31 515return rc;
a0894394 Atul Gupta 2018-03-31
Re: [PATCH 5/7] chtls: free beyond end of array rspq_skb_cache
On Thu, Apr 26, 2018 at 09:52:57AM +0300, Dan Carpenter wrote:
> On Wed, Apr 25, 2018 at 08:36:22PM +0530, Atul Gupta wrote:
> > Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> > Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> > ---
> > drivers/crypto/chelsio/chtls/chtls_main.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c
> > b/drivers/crypto/chelsio/chtls/chtls_main.c
> > index e9ffc3d..4dc3d0e 100644
> > --- a/drivers/crypto/chelsio/chtls/chtls_main.c
> > +++ b/drivers/crypto/chelsio/chtls/chtls_main.c
> > @@ -198,7 +198,7 @@ static void *chtls_uld_add(const struct cxgb4_lld_info
> > *info)
> > {
> > struct cxgb4_lld_info *lldi;
> > struct chtls_dev *cdev;
> > - int i, j;
> > + int i;
> >
> > cdev = kzalloc(sizeof(*cdev) + info->nports *
> > (sizeof(struct net_device *)), GFP_KERNEL);
> > @@ -250,8 +250,8 @@ static void *chtls_uld_add(const struct cxgb4_lld_info
> > *info)
> >
> > return cdev;
> > out_rspq_skb:
> > - for (j = 0; j <= i; j++)
> > - kfree_skb(cdev->rspq_skb_cache[j]);
> > + for (; i > 0; --i)
> > + kfree_skb(cdev->rspq_skb_cache[i]);
>
>
> It should be i >= 0 so that we free cdev->rspq_skb_cache[0].
>
Also, it's still freeing beyond the end of the array... All we needed
to do was change the j <= i in the original to j < i. So it looks like
this:
for (j = 0; j < i; j++)
kfree_skb(cdev->rspq_skb_cache[j]);
regards,
dan carpenter
Re: [PATCH 5/7] chtls: free beyond end of array rspq_skb_cache
On Wed, Apr 25, 2018 at 08:36:22PM +0530, Atul Gupta wrote:
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> ---
> drivers/crypto/chelsio/chtls/chtls_main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c
> b/drivers/crypto/chelsio/chtls/chtls_main.c
> index e9ffc3d..4dc3d0e 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_main.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_main.c
> @@ -198,7 +198,7 @@ static void *chtls_uld_add(const struct cxgb4_lld_info
> *info)
> {
> struct cxgb4_lld_info *lldi;
> struct chtls_dev *cdev;
> - int i, j;
> + int i;
>
> cdev = kzalloc(sizeof(*cdev) + info->nports *
> (sizeof(struct net_device *)), GFP_KERNEL);
> @@ -250,8 +250,8 @@ static void *chtls_uld_add(const struct cxgb4_lld_info
> *info)
>
> return cdev;
> out_rspq_skb:
> - for (j = 0; j <= i; j++)
> - kfree_skb(cdev->rspq_skb_cache[j]);
> + for (; i > 0; --i)
> + kfree_skb(cdev->rspq_skb_cache[i]);
It should be i >= 0 so that we free cdev->rspq_skb_cache[0].
regards,
dan carpenter
Re: [PATCH 4/7] chtls: kbuild warnings
On Wed, Apr 25, 2018 at 08:35:32PM +0530, Atul Gupta wrote:
> - unindented continue
> - check for null page
> - signed return
>
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> ---
> drivers/crypto/chelsio/chtls/chtls_io.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 85ddc07..ce90ba1 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -907,11 +907,11 @@ static int chtls_skb_copy_to_page_nocache(struct sock
> *sk,
> }
>
> /* Read TLS header to find content type and data length */
> -static u16 tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
> +static int tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
> {
> if (copy_from_iter(thdr, sizeof(*thdr), from) != sizeof(*thdr))
> return -EFAULT;
> - return (__force u16)cpu_to_be16(thdr->length);
> + return (__force int)cpu_to_be16(thdr->length);
> }
>
> static int csk_mem_free(struct chtls_dev *cdev, struct sock *sk)
> @@ -1083,6 +1083,9 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg,
> size_t size)
> int off = TCP_OFF(sk);
> bool merge;
>
> + if (!page)
> + goto wait_for_memory;
> +
> if (page)
This second condition isn't necessary.
regards,
dan carpenter
[bug report] crypto: chtls - Program the TLS session Key
Hello Atul Gupta,
The patch d25f2f71f653: "crypto: chtls - Program the TLS session Key"
from Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_hw.c:239 chtls_key_info()
error: '__memcpy()' 'key' too small (2 vs 32)
drivers/crypto/chelsio/chtls/chtls_hw.c
212 static int chtls_key_info(struct chtls_sock *csk,
213struct _key_ctx *kctx,
214u32 keylen, u32 optname)
215 {
216 unsigned char key[CHCR_KEYCTX_CIPHER_KEY_SIZE_256];
^^^
This is 2 bytes long. It was probably supposed to be
AES_KEYSIZE_256 (32 bytes).
217 struct tls12_crypto_info_aes_gcm_128 *gcm_ctx;
218 unsigned char ghash_h[AEAD_H_SIZE];
219 struct crypto_cipher *cipher;
220 int ck_size, key_ctx_size;
221 int ret;
222
223 gcm_ctx = (struct tls12_crypto_info_aes_gcm_128 *)
224>tlshws.crypto_info;
225
226 key_ctx_size = sizeof(struct _key_ctx) +
227 roundup(keylen, 16) + AEAD_H_SIZE;
228
229 if (keylen == AES_KEYSIZE_128) {
230 ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_128;
231 } else if (keylen == AES_KEYSIZE_192) {
232 ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_192;
233 } else if (keylen == AES_KEYSIZE_256) {
^
keylen is 32.
234 ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_256;
235 } else {
236 pr_err("GCM: Invalid key length %d\n", keylen);
237 return -EINVAL;
238 }
239 memcpy(key, gcm_ctx->key, keylen);
^
Memory corruption. Smatch also complains that gcm_ctx->key is 16 bytes
instead of 32.
drivers/crypto/chelsio/chtls/chtls_hw.c:239 chtls_key_info()
error: '__memcpy()' 'gcm_ctx->key' too small (16 vs 32)
240
See also:
drivers/crypto/chelsio/chtls/chtls_hw.c:250 chtls_key_info() error:
'crypto_cipher_setkey()' 'key' too small (2 vs 32)
drivers/crypto/chelsio/chtls/chtls_hw.c:274 chtls_key_info() error:
'__memcpy()' 'gcm_ctx->key' too small (16 vs 32)
drivers/crypto/chelsio/chtls/chtls_hw.c:277 chtls_key_info() error:
'__memset()' 'gcm_ctx->key' too small (16 vs 32)
regards,
dan carpenter
[bug report] crypto: chtls - Inline TLS record Tx
Hello Atul Gupta,
This is a semi-automatic email about new static checker warnings.
The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from
Mar 31, 2018, leads to the following Smatch complaint:
drivers/crypto/chelsio/chtls/chtls_io.c:1156 chtls_sendpage()
warn: variable dereferenced before check 'skb' (see line 1155)
drivers/crypto/chelsio/chtls/chtls_io.c
1154
1155 copy = mss - skb->len;
Dereference
1156 if (!skb || (ULP_SKB_CB(skb)->flags &
ULPCB_FLAG_NO_APPEND) ||
Check
1157 copy <= 0) {
1158 new_buf:
regards,
dan carpenter
[bug report] crypto: chtls - Register chtls with net tls
Hello Atul Gupta,
The patch a08943947873: "crypto: chtls - Register chtls with net tls"
from Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_main.c:447 do_chtls_getsockopt()
warn: check that 'crypto_info.cipher_type' doesn't leak information
drivers/crypto/chelsio/chtls/chtls_main.c
441 static int do_chtls_getsockopt(struct sock *sk, char __user *optval,
442 int __user *optlen)
443 {
444 struct tls_crypto_info crypto_info;
445
446 crypto_info.version = TLS_1_2_VERSION;
447 if (copy_to_user(optval, _info, sizeof(struct
tls_crypto_info)))
448 return -EFAULT;
It is an info leak, but perhaps instead of just zeroing it out we could
set crypto_info.cipher_type to something meaningful?
449 return 0;
450 }
regards,
dan carpenter
[bug report] crypto: chtls - Inline TLS record Rx
Hello Atul Gupta,
The patch b647993fca14: "crypto: chtls - Inline TLS record Rx" from
Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_io.c:1412 chtls_pt_recvmsg()
warn: inconsistent indenting
drivers/crypto/chelsio/chtls/chtls_io.c
1401 if (sk->sk_backlog.tail) {
1402 release_sock(sk);
1403 lock_sock(sk);
1404 chtls_cleanup_rbuf(sk, copied);
1405 continue;
1406 }
1407
1408 if (copied >= target)
1409 break;
1410 chtls_cleanup_rbuf(sk, copied);
1411 sk_wait_data(sk, , NULL);
1412 continue;
The continue is indented too far. Were you intending to check the
return from sk_wait_data() or something?
1413 found_ok_skb:
1414 if (!skb->len) {
1415 skb_dst_set(skb, NULL);
1416 __skb_unlink(skb, >sk_receive_queue);
1417 kfree_skb(skb);
1418
1419 if (!copied && !timeo) {
1420 copied = -EAGAIN;
1421 break;
1422 }
1423
1424 if (copied < target) {
1425 release_sock(sk);
1426 lock_sock(sk);
1427 continue;
1428 }
1429 break;
1430 }
regards,
dan carpenter
[bug report] crypto: chtls - Inline TLS record Tx
Hello Atul Gupta,
The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from
Mar 31, 2018, leads to the following static checker warning:
drivers/crypto/chelsio/chtls/chtls_io.c:913 tls_header_read()
warn: signedness bug returning '(-14)'
drivers/crypto/chelsio/chtls/chtls_io.c
909 /* Read TLS header to find content type and data length */
910 static u16 tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
^^^
911 {
912 if (copy_from_iter(thdr, sizeof(*thdr), from) != sizeof(*thdr))
913 return -EFAULT;
This has a signedness bug and the caller doesn't check for errors.
914 return (__force u16)cpu_to_be16(thdr->length);
915 }
regards,
dan carpenter
[bug report] crypto: chtls - Inline TLS record Tx
Hello Atul Gupta, The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from Mar 31, 2018, leads to the following static checker warning: drivers/crypto/chelsio/chtls/chtls_io.c:1210 chtls_sendpage() info: ignoring unreachable code. drivers/crypto/chelsio/chtls/chtls_io.c 1188 skb->len += copy; 1189 if (skb->len == mss) 1190 tx_skb_finalize(skb); 1191 skb->data_len += copy; 1192 skb->truesize += copy; 1193 sk->sk_wmem_queued += copy; 1194 tp->write_seq += copy; 1195 copied += copy; 1196 offset += copy; 1197 size -= copy; 1198 1199 if (corked(tp, flags) && 1200 (sk_stream_wspace(sk) < sk_stream_min_wspace(sk))) 1201 ULP_SKB_CB(skb)->flags |= ULPCB_FLAG_NO_APPEND; 1202 1203 if (!size) 1204 break; 1205 1206 if (unlikely(ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND)) 1207 push_frames_if_head(sk); 1208 continue; 1209 1210 set_bit(SOCK_NOSPACE, >sk_socket->flags); ^^^ Not reachable. 1211 } 1212 out: 1213 csk_reset_flag(csk, CSK_TX_MORE_DATA); 1214 if (copied) 1215 chtls_tcp_push(sk, flags); 1216 done: 1217 release_sock(sk); 1218 return copied; 1219 1220 do_error: 1221 if (copied) 1222 goto out; 1223 1224 out_err: 1225 if (csk_conn_inline(csk)) regards, dan carpenter
[bug report] crypto: omap-aes - Add support for GCM mode
Hello Tero Kristo,
This is a semi-automatic email about new static checker warnings.
The patch ad18cc9d0f91: "crypto: omap-aes - Add support for GCM mode"
from May 24, 2017, leads to the following Smatch complaint:
drivers/crypto/omap-aes.c:1262 omap_aes_probe()
error: we previously assumed 'dd->pdata->aead_algs_info' could be null (see
line 1237)
drivers/crypto/omap-aes.c
1236
1237 if (dd->pdata->aead_algs_info &&
^
Check for NULL.
1238 !dd->pdata->aead_algs_info->registered) {
1239 for (i = 0; i < dd->pdata->aead_algs_info->size; i++) {
1240 aalg = >pdata->aead_algs_info->algs_list[i];
1241 algp = >base;
1242
1243 pr_debug("reg alg: %s\n", algp->cra_name);
1244 INIT_LIST_HEAD(>cra_list);
1245
1246 err = crypto_register_aead(aalg);
1247 if (err)
1248 goto err_aead_algs;
1249
1250 dd->pdata->aead_algs_info->registered++;
1251 }
1252 }
1253
1254 err = sysfs_create_group(>kobj, _aes_attr_group);
1255 if (err) {
1256 dev_err(dev, "could not create sysfs device attrs\n");
1257 goto err_aead_algs;
^^
Goto. It was actually this goto added in Feb which introduced the
warning, but my scripts got confused.
1258 }
1259
1260 return 0;
1261 err_aead_algs:
1262 for (i = dd->pdata->aead_algs_info->registered - 1; i >= 0;
i--) {
^
Unchecked dereference.
1263 aalg = >pdata->aead_algs_info->algs_list[i];
1264 crypto_unregister_aead(aalg);
regards,
dan carpenter
[PATCH] crypto: chelsio - Delete stray tabs in create_authenc_wr()
We removed some if statements but left these statements indented too far. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c index a9c894bf9c01..34a02d690548 100644 --- a/drivers/crypto/chelsio/chcr_algo.c +++ b/drivers/crypto/chelsio/chcr_algo.c @@ -2112,11 +2112,11 @@ static struct sk_buff *create_authenc_wr(struct aead_request *req, error = chcr_aead_common_init(req, op_type); if (error) return ERR_PTR(error); - dnents = sg_nents_xlen(req->dst, assoclen, CHCR_DST_SG_SIZE, 0); - dnents += sg_nents_xlen(req->dst, req->cryptlen + - (op_type ? -authsize : authsize), CHCR_DST_SG_SIZE, - req->assoclen); - dnents += MIN_AUTH_SG; // For IV + dnents = sg_nents_xlen(req->dst, assoclen, CHCR_DST_SG_SIZE, 0); + dnents += sg_nents_xlen(req->dst, req->cryptlen + + (op_type ? -authsize : authsize), CHCR_DST_SG_SIZE, + req->assoclen); + dnents += MIN_AUTH_SG; // For IV dst_size = get_space_for_phys_dsgl(dnents); kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) << 4)
[bug report] crypto: lrw - Convert to skcipher
Hello Herbert Xu,
The patch 700cb3f5fe75: "crypto: lrw - Convert to skcipher" from Nov
22, 2016, leads to the following static checker warning:
crypto/lrw.c:316 exit_crypt()
warn: should '(struct rctx)->ext' be freed with kzfree()'
crypto/lrw.c
309 static void exit_crypt(struct skcipher_request *req)
310 {
311 struct rctx *rctx = skcipher_request_ctx(req);
312
313 rctx->left = 0;
314
315 if (rctx->ext)
316 kfree(rctx->ext);
I am working on a Smatch check that complains about stuff we should
maybe free with kzfree. It first makes a list of any struct members
which are freed with kzfree() then it does a second pass and complains
if any of them are freed with regular kfree().
317 }
Here is the complete list of warnings from v4.15-rc8. It's not very
long...
crypto/lrw.c:316 exit_crypt() warn: should '(struct rctx)->ext' be freed with
kzfree()'
drivers/crypto/virtio/virtio_crypto_core.c:411 virtcrypto_free_unused_reqs()
warn: should '(struct virtio_crypto_request)->req_data' be freed with kzfree()'
drivers/net/wireless/intersil/orinoco/wext.c:78 orinoco_set_key() warn: should
'(struct key_params)->key' be freed with kzfree()'
drivers/staging/wlan-ng/p80211conv.c:216 skb_ether_to_p80211() warn: should
'(struct p80211_metawep)->data' be freed with kzfree()'
fs/cifs/connect.c:1710 cifs_parse_mount_options() warn: should '(struct
smb_vol)->password' be freed with kzfree()'
fs/cifs/connect.c:1748 cifs_parse_mount_options() warn: should '(struct
smb_vol)->password' be freed with kzfree()'
fs/cifs/connect.c:4238 cifs_construct_tcon() warn: should '(struct
smb_vol)->password' be freed with kzfree()'
security/apparmor/crypto.c:102 aa_calc_profile_hash() warn: should '(struct
aa_profile)->hash' be freed with kzfree()'
regards,
dan carpenter
[bug report] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
Hello Brijesh Singh,
The patch 200664d5237f: "crypto: ccp: Add Secure Encrypted
Virtualization (SEV) command support" from Dec 4, 2017, leads to the
following static checker warning:
drivers/crypto/ccp/psp-dev.c:779 psp_pci_init()
error: uninitialized symbol 'error'.
drivers/crypto/ccp/psp-dev.c
764 void psp_pci_init(void)
765 {
766 struct sev_user_data_status *status;
767 struct sp_device *sp;
768 int error, rc;
769
770 sp = sp_get_psp_master_device();
771 if (!sp)
772 return;
773
774 psp_master = sp->psp_data;
775
776 /* Initialize the platform */
777 rc = sev_platform_init();
778 if (rc) {
779 dev_err(sp->dev, "SEV: failed to INIT error %#x\n",
error);
^
Generally not set on the error paths.
780 goto err;
781 }
782
783 /* Display SEV firmware version */
regards,
dan carpenter
[PATCH] staging: ccree: don't break lines unnecessarily
These lines are less than 80 characters so we don't need to break them
up into chunks.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/staging/ccree/cc_aead.c b/drivers/staging/ccree/cc_aead.c
index 265adffdab41..b58413172231 100644
--- a/drivers/staging/ccree/cc_aead.c
+++ b/drivers/staging/ccree/cc_aead.c
@@ -2600,8 +2600,7 @@ static struct cc_crypto_alg *cc_create_aead_alg(struct
cc_alg_template *tmpl,
alg = >template_aead;
- snprintf(alg->base.cra_name, CRYPTO_MAX_ALG_NAME, "%s",
-tmpl->name);
+ snprintf(alg->base.cra_name, CRYPTO_MAX_ALG_NAME, "%s", tmpl->name);
snprintf(alg->base.cra_driver_name, CRYPTO_MAX_ALG_NAME, "%s",
tmpl->driver_name);
alg->base.cra_module = THIS_MODULE;
diff --git a/drivers/staging/ccree/cc_cipher.c
b/drivers/staging/ccree/cc_cipher.c
index 8afdbc120b13..5c7e91f1cde7 100644
--- a/drivers/staging/ccree/cc_cipher.c
+++ b/drivers/staging/ccree/cc_cipher.c
@@ -127,8 +127,7 @@ static int validate_data_size(struct cc_cipher_ctx *ctx_p,
static unsigned int get_max_keysize(struct crypto_tfm *tfm)
{
struct cc_crypto_alg *cc_alg =
- container_of(tfm->__crt_alg, struct cc_crypto_alg,
-crypto_alg);
+ container_of(tfm->__crt_alg, struct cc_crypto_alg, crypto_alg);
if ((cc_alg->crypto_alg.cra_flags & CRYPTO_ALG_TYPE_MASK) ==
CRYPTO_ALG_TYPE_ABLKCIPHER)
@@ -391,8 +390,7 @@ static void cc_setup_cipher_desc(struct crypto_tfm *tfm,
unsigned int du_size = nbytes;
struct cc_crypto_alg *cc_alg =
- container_of(tfm->__crt_alg, struct cc_crypto_alg,
-crypto_alg);
+ container_of(tfm->__crt_alg, struct cc_crypto_alg, crypto_alg);
if ((cc_alg->crypto_alg.cra_flags & CRYPTO_ALG_BULK_MASK) ==
CRYPTO_ALG_BULK_DU_512)
@@ -611,8 +609,7 @@ static void cc_cipher_complete(struct device *dev, void
*cc_req, int err)
kfree(req_ctx->backup_info);
} else if (!err) {
scatterwalk_map_and_copy(req->info, req->dst,
-(req->nbytes - ivsize),
-ivsize, 0);
+(req->nbytes - ivsize), ivsize, 0);
}
ablkcipher_request_complete(areq, err);
@@ -1096,8 +1093,7 @@ struct cc_crypto_alg *cc_cipher_create_alg(struct
cc_alg_template *template,
int cc_cipher_free(struct cc_drvdata *drvdata)
{
struct cc_crypto_alg *t_alg, *n;
- struct cc_cipher_handle *blkcipher_handle =
- drvdata->blkcipher_handle;
+ struct cc_cipher_handle *blkcipher_handle = drvdata->blkcipher_handle;
if (blkcipher_handle) {
/* Remove registered algs */
list_for_each_entry_safe(t_alg, n,
diff --git a/drivers/staging/ccree/cc_driver.c
b/drivers/staging/ccree/cc_driver.c
index 6682d9d93931..192b1759de45 100644
--- a/drivers/staging/ccree/cc_driver.c
+++ b/drivers/staging/ccree/cc_driver.c
@@ -216,8 +216,7 @@ static int init_cc_resources(struct platform_device
*plat_dev)
}
if (rc) {
- dev_err(dev, "Failed in dma_set_mask, mask=%par\n",
- _mask);
+ dev_err(dev, "Failed in dma_set_mask, mask=%par\n", _mask);
return rc;
}
diff --git a/drivers/staging/ccree/cc_fips.c b/drivers/staging/ccree/cc_fips.c
index b25c34e08717..de08af976b7f 100644
--- a/drivers/staging/ccree/cc_fips.c
+++ b/drivers/staging/ccree/cc_fips.c
@@ -53,8 +53,7 @@ void cc_fips_fini(struct cc_drvdata *drvdata)
void fips_handler(struct cc_drvdata *drvdata)
{
- struct cc_fips_handle *fips_handle_ptr =
- drvdata->fips_handle;
+ struct cc_fips_handle *fips_handle_ptr = drvdata->fips_handle;
tasklet_schedule(_handle_ptr->tasklet);
}
diff --git a/drivers/staging/ccree/cc_hash.c b/drivers/staging/ccree/cc_hash.c
index 86f9ec711edc..8afc39f10bb3 100644
--- a/drivers/staging/ccree/cc_hash.c
+++ b/drivers/staging/ccree/cc_hash.c
@@ -1858,9 +1858,8 @@ int cc_init_hash_sram(struct cc_drvdata *drvdata)
hash_handle->larval_digest_sram_addr = sram_buff_ofs;
/* Copy-to-sram initial SHA* digests */
- cc_set_sram_desc(md5_init, sram_buff_ofs,
-ARRAY_SIZE(md5_init), larval_seq,
-_seq_len);
+ cc_set_sram_desc(md5_init, sram_buff_ofs, ARRAY_SIZE(md5_init),
+larval_seq, _seq_len);
rc = send_request_init(drvdata, larval_seq, larval_seq_len);
if (rc)
goto init_digest_const_err;
@@ -2004,8 +2003,7 @@ int cc_hash_alloc(struct cc_drvdata *drvdata)
Re: Getting the ccree driver out of staging
Here are my remaining Smatch warnings:
drivers/staging/ccree/cc_driver.c:219 init_cc_resources()
error: '%pa' can only be followed by one of [dp]
drivers/staging/ccree/cc_driver.c
217
218 if (rc) {
219 dev_err(dev, "Failed in dma_set_mask, mask=%par\n",
^
This 'r' is is treated as a 'p'. Not sure what was intended.
220 _mask);
221 return rc;
222 }
223
drivers/staging/ccree/cc_buffer_mgr.c:1067 cc_aead_chain_data()
warn: inconsistent indenting
drivers/staging/ccree/cc_buffer_mgr.c
1064 if (src_mapped_nents > LLI_MAX_NUM_OF_DATA_ENTRIES) {
1065 dev_err(dev, "Too many fragments. current %d max %d\n",
1066 src_mapped_nents, LLI_MAX_NUM_OF_DATA_ENTRIES);
1067 return -ENOMEM;
^^
1068 }
drivers/staging/ccree/cc_cipher.c:373 cc_cipher_setkey()
warn: inconsistent indenting
drivers/staging/ccree/cc_cipher.c
369 dma_sync_single_for_device(dev, ctx_p->user.key_dma_addr,
370 max_key_buf_size, DMA_TO_DEVICE);
371 ctx_p->keylen = keylen;
372
373 dev_dbg(dev, "return safely");
^
One extra space.
374 return 0;
375 }
regards,
dan carpenter
[PATCH] hwrng: exynos - Signedness bug in exynos_trng_do_read()
"val" needs to be signed for the error handling to work.
Fixes: 6cd225cc5d8a ("hwrng: exynos - add Samsung Exynos True RNG driver")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/char/hw_random/exynos-trng.c
b/drivers/char/hw_random/exynos-trng.c
index 34d6f51ecbee..f4643e3ec346 100644
--- a/drivers/char/hw_random/exynos-trng.c
+++ b/drivers/char/hw_random/exynos-trng.c
@@ -55,7 +55,7 @@ static int exynos_trng_do_read(struct hwrng *rng, void *data,
size_t max,
bool wait)
{
struct exynos_trng_dev *trng;
- u32 val;
+ int val;
max = min_t(size_t, max, (EXYNOS_TRNG_FIFO_LEN * 4));
Re: [PATCH 01/10] staging: ccree: remove inline qualifiers
On Thu, Dec 07, 2017 at 09:00:11AM +0200, Gilad Ben-Yossef wrote: > On Mon, Dec 4, 2017 at 11:36 AM, Dan Carpenter <dan.carpen...@oracle.com> > wrote: > > On Sun, Dec 03, 2017 at 01:58:12PM +, Gilad Ben-Yossef wrote: > >> The ccree drivers was marking a lot of big functions in C file as > >> static inline for no good reason. Remove the inline qualifier from > >> any but the few truly single line functions. > >> > > > > The compiler is free to ignore inline hints... It probably would make > > single line functions inline anyway. > > > > Yes. I think of it more as a note to the reader: "don't add stuff to > this function. it is meant to be short and simple". > Ah. Fine. regards, dan carpenter
[bug report] chcr: Add support for Inline IPSec
Hello Atul Gupta,
The patch 6dad4e8ab3ec: "chcr: Add support for Inline IPSec" from Nov
16, 2017, leads to the following static checker warning:
drivers/crypto/chelsio/chcr_ipsec.c:431 copy_key_cpltx_pktxt()
warn: potential pointer math issue ('q->q.desc' is a 512 bit pointer)
drivers/crypto/chelsio/chcr_ipsec.c
419
420 if (likely(len <= left)) {
421 memcpy(key_ctx->key, sa_entry->key, key_len);
422 pos += key_len;
423 } else {
424 if (key_len <= left) {
425 memcpy(pos, sa_entry->key, key_len);
426 pos += key_len;
427 } else {
428 memcpy(pos, sa_entry->key, left);
429 memcpy(q->q.desc, sa_entry->key + left,
430 key_len - left);
431 pos = q->q.desc + (key_len - left);
^
This does look like a pointer math issue. It should probably be:
pos = (u8 *)q->q.desc + (key_len - left);
But I can't test this.
432 }
433 }
434 /* Copy CPL TX PKT XT */
435 pos = copy_cpltx_pktxt(skb, dev, pos);
regards,
dan carpenter
Re: [PATCH 01/10] staging: ccree: remove inline qualifiers
On Sun, Dec 03, 2017 at 01:58:12PM +, Gilad Ben-Yossef wrote: > The ccree drivers was marking a lot of big functions in C file as > static inline for no good reason. Remove the inline qualifier from > any but the few truly single line functions. > The compiler is free to ignore inline hints... It probably would make single line functions inline anyway. regards, dan carpenter
Re: [PATCH 00/10] staging: ccree: cleanups & fixes
Looks good. Thanks! regards, dan carpenter
Re: [PATCH 00/24] staging: ccree: more cleanup patches
On Tue, Nov 14, 2017 at 11:33:20AM +0200, Gilad Ben-Yossef wrote: > On Mon, Nov 13, 2017 at 8:33 PM, Dan Carpenter <dan.carpen...@oracle.com> > wrote: > > These cleanups look nice. Thanks. > > > > I hope you do a mass remove of likely/unlikely in a patch soon. > > Whenever, I see one of those in a + line I always have to remind myself > > that you're planning to do it in a later patch. > > > > So, a question about that - there indeed seems to be an inflation of > likely/unlikely in the ccree driver, but > what stopped me from removing them was that I found out I don't have a > clue about when it's a good idea > to use them and when it isn't (obviously in places where you know the > probable code flow of course). > > Any hints? They should only be included if benchmarking shows that it makes a difference. I think they need to be about 100 right predictions to 1 wrong prediction on a fast path. So remove them all and add them back one at a time. regards, dan carpenter
Re: [PATCH 00/24] staging: ccree: more cleanup patches
These cleanups look nice. Thanks. I hope you do a mass remove of likely/unlikely in a patch soon. Whenever, I see one of those in a + line I always have to remind myself that you're planning to do it in a later patch. regards, dan carpenter
[PATCH] crypto: s5p-sss - Remove a stray tab
This code seems correct, but the goto was indented too far.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 142c6020cec7..62830a43d959 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -1461,7 +1461,7 @@ static void s5p_hash_tasklet_cb(unsigned long data)
>hash_flags)) {
/* hash or semi-hash ready */
clear_bit(HASH_FLAGS_DMA_READY, >hash_flags);
- goto finish;
+ goto finish;
}
}
[PATCH] crypto: chelsio - Fix an error code in chcr_hash_dma_map()
The dma_map_sg() function returns zero on error and positive values on
success. We want to return -ENOMEM on failure here and zero on success.
Fixes: 2f47d5804311 ("crypto: chelsio - Move DMA un/mapping to chcr from lld
cxgb4 driver")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/chelsio/chcr_algo.c
b/drivers/crypto/chelsio/chcr_algo.c
index 4eed7171e2ae..38fe59b5c689 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2414,7 +2414,7 @@ static inline int chcr_hash_dma_map(struct device *dev,
error = dma_map_sg(dev, req->src, sg_nents(req->src),
DMA_TO_DEVICE);
if (!error)
- return error;
+ return -ENOMEM;
req_ctx->is_sg_map = 1;
return 0;
}
Re: [PATCH v2 00/10] staging: ccree: fixes and cleanups
Reviewed-by: Dan Carpenter <dan.carpen...@oracle.com> regards, dan carpenter
Re: [PATCH 6/8] staging: ccree: simplify pm manager using local var
On Thu, Nov 09, 2017 at 08:27:28AM +0200, Gilad Ben-Yossef wrote:
> On Tue, Nov 7, 2017 at 12:43 PM, Dan Carpenter <dan.carpen...@oracle.com>
> wrote:
> > On Tue, Nov 07, 2017 at 09:40:02AM +, Gilad Ben-Yossef wrote:
> >> --- a/drivers/staging/ccree/ssi_pm.c
> >> +++ b/drivers/staging/ccree/ssi_pm.c
> >> @@ -90,20 +90,24 @@ int cc_pm_resume(struct device *dev)
> >> int cc_pm_get(struct device *dev)
> >> {
> >> int rc = 0;
> >> + struct ssi_drvdata *drvdata =
> >> + (struct ssi_drvdata *)dev_get_drvdata(dev);
> >
> > No need to cast:
> >
> > struct ssi_drvdata *drvdata = dev_get_drvdata(dev);
> >
>
> The same unneeded cast appears at other places in the file, so I opted
> to add a patch addressing all these location rather then change this one.
>
> I hope it's OK.
I don't care about this one patch, it's fine. But generally and for
future reference, we don't try very hard to use a consistent style
within a driver. The preference is almost always kernel style over
driver style so new code should always be "kernel style" where
we avoid casting from kmalloc() or other functions that return void
pointers.
regards,
dan carpenter
[bug report] crypto: chelsio - Move DMA un/mapping to chcr from lld cxgb4 driver
Hello Harsh Jain,
This is a semi-automatic email about new static checker warnings.
The patch 2f47d5804311: "crypto: chelsio - Move DMA un/mapping to
chcr from lld cxgb4 driver" from Oct 8, 2017, leads to the following
Smatch complaint:
drivers/crypto/chelsio/chcr_algo.c:562 ulptx_walk_add_sg()
error: we previously assumed 'sg' could be null (see line 551)
drivers/crypto/chelsio/chcr_algo.c
550
551 while (sg && skip) {
^^
The patch adds a new check for NULL
552 if (sg_dma_len(sg) <= skip) {
553 skip -= sg_dma_len(sg);
554 skip_len = 0;
555 sg = sg_next(sg);
556 } else {
557 skip_len = skip;
558 skip = 0;
559 }
560 }
561 if (walk->nents == 0) {
562 small = min_t(unsigned int, sg_dma_len(sg) - skip_len,
len);
^^
This dereference (inside the macro) isn't checked.
563 sgmin = min_t(unsigned int, small, CHCR_SRC_SG_SIZE);
564 walk->sgl->len0 = cpu_to_be32(sgmin);
regards,
dan carpenter
Re: [PATCH 7/8] staging: ccree: remove compare to none zero
On Tue, Nov 07, 2017 at 09:40:03AM +, Gilad Ben-Yossef wrote:
> diff --git a/drivers/staging/ccree/ssi_aead.c
> b/drivers/staging/ccree/ssi_aead.c
> index f1a3976..e9d03ee 100644
> --- a/drivers/staging/ccree/ssi_aead.c
> +++ b/drivers/staging/ccree/ssi_aead.c
> @@ -240,7 +240,7 @@ static void ssi_aead_complete(struct device *dev, void
> *ssi_req, void __iomem *c
>
> if (areq_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) {
> if (memcmp(areq_ctx->mac_buf, areq_ctx->icv_virt_addr,
> -ctx->authsize) != 0) {
> +ctx->authsize)) {
Keep the != for *cmp functions. It makes it way more readable. The
idiom is:
if (memcmp(a, b, size) != 0) <-- this means a != b
if (memcmp(a, b, size) < 0) <-- this means a < b
if (memcmp(a, b, size) == 0) <-- this means a == b
> dev_dbg(dev, "Payload authentication failure,
> (auth-size=%d, cipher=%d)\n",
> ctx->authsize, ctx->cipher_mode);
> /* In case of payload authentication failure, MUST NOT
> @@ -458,7 +458,7 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8
> *key, unsigned int keyl
> hashmode = DRV_HASH_HW_SHA256;
> }
>
> - if (likely(keylen != 0)) {
> + if (likely(keylen)) {
You can keep the zero here as well if you want. keylen is a number and
zero is a number. If you want to remove it that's fine too.
> key_dma_addr = dma_map_single(dev, (void *)key, keylen,
> DMA_TO_DEVICE);
> if (unlikely(dma_mapping_error(dev, key_dma_addr))) {
> dev_err(dev, "Mapping key va=0x%p len=%u for DMA
> failed\n",
> @@ -517,7 +517,7 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8
> *key, unsigned int keyl
> keylen, NS_BIT, 0);
> idx++;
>
> - if ((blocksize - keylen) != 0) {
> + if (blocksize - keylen) {
Same. Numbers can be compared to zero and it's fine.
> hw_desc_init([idx]);
> set_din_const([idx], 0,
> (blocksize - keylen));
> @@ -539,10 +539,10 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const
> u8 *key, unsigned int keyl
> }
>
> rc = send_request(ctx->drvdata, _req, desc, idx, 0);
> - if (unlikely(rc != 0))
> + if (unlikely(rc))
Where-as for these ones "rc" is not a number we can use for math so the
!= 0 is just a double negative and slightly confusing, so removing it
is the right thing.
regards,
dan carpenter
Re: [PATCH 6/8] staging: ccree: simplify pm manager using local var
On Tue, Nov 07, 2017 at 09:40:02AM +, Gilad Ben-Yossef wrote:
> --- a/drivers/staging/ccree/ssi_pm.c
> +++ b/drivers/staging/ccree/ssi_pm.c
> @@ -90,20 +90,24 @@ int cc_pm_resume(struct device *dev)
> int cc_pm_get(struct device *dev)
> {
> int rc = 0;
> + struct ssi_drvdata *drvdata =
> + (struct ssi_drvdata *)dev_get_drvdata(dev);
No need to cast:
struct ssi_drvdata *drvdata = dev_get_drvdata(dev);
regards,
dan carpenter
Re: [PATCH 5/8] staging: ccree: fold common code into function
On Tue, Nov 07, 2017 at 09:40:01AM +, Gilad Ben-Yossef wrote:
> @@ -669,21 +690,12 @@ void cc_unmap_aead_request(struct device *dev, struct
> aead_request *req)
> }
> if (drvdata->coherent &&
> (areq_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) &&
> - likely(req->src == req->dst)) {
> - u32 size_to_skip = req->assoclen;
> -
> - if (areq_ctx->is_gcm4543)
> - size_to_skip += crypto_aead_ivsize(tfm);
> + likely(req->src == req->dst))
Don't remove the curly braces from this one. Multi-line indents get
curly braces for readability.
>
> - /* copy mac to a temporary location to deal with possible
> + /* copy back mac from temporary location to deal with possible
>* data memory overriding that caused by cache coherence
> problem.
>*/
> - cc_copy_sg_portion(dev, areq_ctx->backup_mac, req->src,
> -(size_to_skip + req->cryptlen -
> - areq_ctx->req_authsize),
> -(size_to_skip + req->cryptlen),
> - SSI_SG_FROM_BUF);
> - }
> + cc_copy_mac(dev, req, SSI_SG_FROM_BUF);
> }
regards,
dan carpenter
Re: [PATCH 3/8] staging: ccree: simplify AEAD using local var
On Tue, Nov 07, 2017 at 09:39:59AM +, Gilad Ben-Yossef wrote:
> Make the code more readable by using a local variable
> for commonly use expression in the AEAD part of the driver.
>
> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
> ---
> drivers/staging/ccree/ssi_aead.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_aead.c
> b/drivers/staging/ccree/ssi_aead.c
> index 0b5b230..f1a3976 100644
> --- a/drivers/staging/ccree/ssi_aead.c
> +++ b/drivers/staging/ccree/ssi_aead.c
> @@ -251,13 +251,11 @@ static void ssi_aead_complete(struct device *dev, void
> *ssi_req, void __iomem *c
> }
> } else { /*ENCRYPT*/
> if (unlikely(areq_ctx->is_icv_fragmented)) {
> + u32 loc = areq->cryptlen + areq_ctx->dst_offset;
"loc" isn't a very canonical name. At first I thought this was "pos"
or maybe "end" but now I'm thinking this is "skip"? I don't know what
this variable is.
> +
> cc_copy_sg_portion(dev, areq_ctx->mac_buf,
> -areq_ctx->dst_sgl,
> -(areq->cryptlen +
> - areq_ctx->dst_offset),
> -(areq->cryptlen +
> - areq_ctx->dst_offset +
> - ctx->authsize),
> +areq_ctx->dst_sgl, loc,
> +(loc + ctx->authsize),
> SSI_SG_FROM_BUF);
> }
regards,
dan carpenter
Re: [PATCH 2/8] staging: ccree: use more readable func names
On Tue, Nov 07, 2017 at 09:39:58AM +, Gilad Ben-Yossef wrote: > @@ -780,11 +766,10 @@ static inline int ssi_buffer_mgr_aead_chain_iv( > unsigned int iv_size_to_authenc = crypto_aead_ivsize(tfm); > unsigned int iv_ofs = GCM_BLOCK_RFC4_IV_OFFSET; > /* Chain to given list */ > - ssi_buffer_mgr_add_buffer_entry( > - dev, sg_data, > - areq_ctx->gen_ctx.iv_dma_addr + iv_ofs, > - iv_size_to_authenc, is_last, > - _ctx->assoc.mlli_nents); > + cc_add_buffer_entry(dev, sg_data, > + (areq_ctx->gen_ctx.iv_dma_addr + iv_ofs), ^ ^ No need to resend the patch, but you've added parenthesis here that weren't in the original code and in other places you fixed up some long line warnings. I'd prefer if you didn't do that because I use scripts to review rename patches automatically and since these are not a rename so it means I have to review them manually. Also in that patch that Tobin complained about you had some extra comment changes and some were related to the API but some were just reformatted to fit in 80 chars. In this sample, you've re-indented the code a bit to align correctly. That would be mandatory especially if the original code had aligned correctly so that's fine. But otherwise try to be strict about the one thing per patch. > + iv_size_to_authenc, is_last, > + _ctx->assoc.mlli_nents); > areq_ctx->assoc_buff_type = SSI_DMA_BUF_MLLI; > } > regards, dan carpenter
Re: [PATCH 2/3] staging: ccree: handle limiting of DMA masks
On Tue, Oct 31, 2017 at 11:56:16AM +, Gilad Ben-Yossef wrote:
>
> - if (!dev->coherent_dma_mask)
> - dev->coherent_dma_mask = DMA_BIT_MASK(DMA_BIT_MASK_LEN);
> + if (rc) {
> + dev_err(dev, "Error: failed in dma_set_mask, mask=%par\n",
> + _mask);
> + goto post_drvdata_err;
Also this is not the right goto. We should be turning the clk off.
I don't really care for the naming scheme, and I know you renamed it for
me already once and I feel bad for not liking it more. It's still
really a come-from label name and doesn't say what the goto does...
Instead of post_clk_err, I wish it had names like "err_clk_off:".
And in this case what it does is print a duplicative error message and
return. :/ The goto post_drvdata_err: lines should just print their
own error messages if needed and return directly. If there is no
cleanup then there is no need for a goto.
Anyway, that's not related to this patch. Just resend it with
goto post_clk_err: in the v2.
regards,
dan carpenter
Re: [PATCH 2/3] staging: ccree: handle limiting of DMA masks
On Tue, Oct 31, 2017 at 11:56:16AM +, Gilad Ben-Yossef wrote:
> + dma_mask = (dma_addr_t)(DMA_BIT_MASK(DMA_BIT_MASK_LEN));
> + while (dma_mask > 0x7fffUL) {
> + if (dma_supported(_dev->dev, dma_mask)) {
> + rc = dma_set_coherent_mask(_dev->dev, dma_mask);
> + if (!rc)
> + break;
The indenting is messed up.
> + }
> + dma_mask >>= 1;
> + }
regards,
dan carpenter
Re: [PATCH 1/3] staging: ccree: copy IV to DMAable memory
On Tue, Oct 31, 2017 at 11:56:15AM +, Gilad Ben-Yossef wrote: > + > + /* The IV we are handed may be allocted from the stack so > + * we must copy it to a DMAable buffer before use. > + */ > + req_ctx->iv = kmalloc(ivsize, GFP_KERNEL); > + memcpy(req_ctx->iv, info, ivsize); We need to check if kmalloc() fails. regards, dan carpenter
Re: [PATCH] staging: ccree: Fix indentation in ssi_buffer_mgr.c
On Thu, Oct 26, 2017 at 06:53:42PM -0700, Stephen Brennan wrote:
> In particular, fixes some over-indented if statement bodies as well as a
> couple lines indented with spaces. checkpatch.pl now reports no warnings
> on this file other than 80 character warnings.
>
> Signed-off-by: Stephen Brennan <step...@brennan.io>
> ---
> Hello again, hoping these indentation issues are a bit more actionable than
> the
> line length errors I was trying to fix in my previous patch. Again, thanks in
> advance for taking the time to look!
>
> drivers/staging/ccree/ssi_buffer_mgr.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c
> b/drivers/staging/ccree/ssi_buffer_mgr.c
> index dca3ce84d043..7c62255128d5 100644
> --- a/drivers/staging/ccree/ssi_buffer_mgr.c
> +++ b/drivers/staging/ccree/ssi_buffer_mgr.c
> @@ -406,8 +406,8 @@ ssi_aead_handle_config_buf(struct device *dev,
> sg_init_one(_ctx->ccm_adata_sg, config_data, AES_BLOCK_SIZE +
> areq_ctx->ccm_hdr_size);
> if (unlikely(dma_map_sg(dev, _ctx->ccm_adata_sg, 1,
> DMA_TO_DEVICE) != 1)) {
> - dev_err(dev, "dma_map_sg() config buffer failed\n");
> - return -ENOMEM;
> + dev_err(dev, "dma_map_sg() config buffer failed\n");
> + return -ENOMEM;
> }
> dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK
> offset=%u length=%u\n",
> _dma_address(_ctx->ccm_adata_sg),
> @@ -435,8 +435,8 @@ static inline int ssi_ahash_handle_curr_buf(struct device
> *dev,
> sg_init_one(areq_ctx->buff_sg, curr_buff, curr_buff_cnt);
> if (unlikely(dma_map_sg(dev, areq_ctx->buff_sg, 1,
> DMA_TO_DEVICE) != 1)) {
> - dev_err(dev, "dma_map_sg() src buffer failed\n");
> - return -ENOMEM;
> + dev_err(dev, "dma_map_sg() src buffer failed\n");
> + return -ENOMEM;
So far so good.
> }
> dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK
> offset=%u length=%u\n",
> _dma_address(areq_ctx->buff_sg), sg_page(areq_ctx->buff_sg),
> @@ -1029,10 +1029,10 @@ static inline int
> ssi_buffer_mgr_prepare_aead_data_mlli(
>* verification is made by CPU compare in order to
> simplify
>* MAC verification upon request completion
>*/
> - u32 size_to_skip = req->assoclen;
> + u32 size_to_skip = req->assoclen;
>
> - if (areq_ctx->is_gcm4543)
> - size_to_skip += crypto_aead_ivsize(tfm);
> + if (areq_ctx->is_gcm4543)
> + size_to_skip += crypto_aead_ivsize(tfm);
>
> ssi_buffer_mgr_copy_scatterlist_portion(
> dev, areq_ctx->backup_mac, req->src,
But then ssi_buffer_mgr_copy_scatterlist_portion() is still not indented
correctly.
regards,
dan carpenter
Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock
On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote:
> The use of volatile for the variable monitor_lock is unnecessary.
>
> Signed-off-by: Srishti Sharma <srishtis...@gmail.com>
> ---
> drivers/staging/ccree/ssi_request_mgr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ccree/ssi_request_mgr.c
> b/drivers/staging/ccree/ssi_request_mgr.c
> index e5c2f92..7d77941 100644
> --- a/drivers/staging/ccree/ssi_request_mgr.c
> +++ b/drivers/staging/ccree/ssi_request_mgr.c
> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
> dma_addr_t dummy_comp_buff_dma;
> struct cc_hw_desc monitor_desc;
>
> - volatile unsigned long monitor_lock;
> + unsigned long monitor_lock;
The variable seems unused. Try deleting it instead.
regards,
dan carpenter
Re: [PATCH 7/8] staging: ccree: replace noop macro with inline
On Sun, Sep 03, 2017 at 11:56:49AM +0300, Gilad Ben-Yossef wrote:
> Replace noop macro with a noop inline function
>
> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
> ---
> drivers/staging/ccree/ssi_driver.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_driver.h
> b/drivers/staging/ccree/ssi_driver.h
> index 06a3c48..81ba827 100644
> --- a/drivers/staging/ccree/ssi_driver.h
> +++ b/drivers/staging/ccree/ssi_driver.h
> @@ -187,8 +187,8 @@ struct async_gen_req_ctx {
> #ifdef DX_DUMP_BYTES
> void dump_byte_array(const char *name, const u8 *the_array, unsigned long
> size);
> #else
> -#define dump_byte_array(name, array, size) do { \
> -} while (0);
> +static inline void dump_byte_array(const char *name, const u8 *the_array,
> +unsigned long size) {};
Could you put the {} on the next line? Also there is no need for the
semi-colon after the end of a function.
This is a style thing, so if you want to do it in a follow on patch
that's fine
regards,
dan carpenter
Re: [PATCH v5] Staging: ccree: Remove unused variable.
Looks good. Thanks! regards, dan carpenter
Re: [PATCH v3] Staging: ccree: ssi_cipher.c: Remove unused variable.
Always compile your patches. CC [M] drivers/staging/ccree/ssi_cipher.o drivers/staging/ccree/ssi_cipher.c: In function ‘ssi_blkcipher_complete’: drivers/staging/ccree/ssi_cipher.c:700:6: warning: unused variable ‘inflight_counter’ [-Wunused-variable] u32 inflight_counter; ^~~~ You need to delete the declaration as well. Don't be in a rush to resend patches. I normally write them then let them sit in my outbox overnight and send them in the morning. The extra delay helps me to calm down a bit and focus better. Even though I've sent thousands of patches, it sometimes still stresses me out. It's like you're disagreeing with the original author and the reviewers are disagreeing with you and everyone's trying to be nice about it but patches are fundamentally points of disagreement and that's stress. regards, dan carpenter
Re: [PATCH 8/8] staging: ccree: remove BUG macro usage
On Sun, Sep 03, 2017 at 11:56:50AM +0300, Gilad Ben-Yossef wrote:
> @@ -1154,7 +1150,8 @@ static inline int ssi_buffer_mgr_aead_chain_data(
> //if have reached the end of the sgl, then this is unexpected
> if (!areq_ctx->src_sgl) {
> SSI_LOG_ERR("reached end of sg list. unexpected\n");
> - BUG();
> + return -EINVAL;
> + goto chain_data_exit;
You've got a direct return followed by a goto.
It's a do-nothing goto that just returns rc. I hate those. I've tried
to review locking bugs to see if single returns prevent future
programmers from introducing new error paths which don't unlock. They
don't really help...
regards,
dan carpenter
Re: [PATCH] staging/ccree: Declare compiled out fuctions static inline
Try saving this email and then do `cat email.txt | git am`. It is corrupted. This is not how to send a v2 patch. Please read: https://kernelnewbies.org/FirstKernelPatch#head-5c81b3c517a1d0bbc24f92594cb734e155fcbbcb Look through the email archives for examples of other v2 patches. regards, dan carpenter
Re: [PATCH] staging/ccree: Declare compiled out fuctions static inline
Google for how to send a v2 patch. https://www.google.com/search?q=how+to+send+a+v2+patch https://kernelnewbies.org/FirstKernelPatch regards, dan carpenter
[bug report] crypto: stm32 - Support for STM32 HASH module
Hello lionel.debi...@st.com,
The patch 8a1012d3f2ab: "crypto: stm32 - Support for STM32 HASH
module" from Jul 13, 2017, leads to the following static checker
warning:
drivers/crypto/stm32/stm32-hash.c:1088 stm32_hash_irq_thread()
error: uninitialized symbol 'err'.
drivers/crypto/stm32/stm32-hash.c
1067 static irqreturn_t stm32_hash_irq_thread(int irq, void *dev_id)
1068 {
1069 struct stm32_hash_dev *hdev = dev_id;
1070 int err;
1071
1072 if (HASH_FLAGS_CPU & hdev->flags) {
1073 if (HASH_FLAGS_OUTPUT_READY & hdev->flags) {
1074 hdev->flags &= ~HASH_FLAGS_OUTPUT_READY;
1075 goto finish;
1076 }
1077 } else if (HASH_FLAGS_DMA_READY & hdev->flags) {
1078 if (HASH_FLAGS_DMA_ACTIVE & hdev->flags) {
1079 hdev->flags &= ~HASH_FLAGS_DMA_ACTIVE;
1080 goto finish;
1081 }
1082 }
1083
1084 return IRQ_HANDLED;
1085
1086 finish:
1087 /*Finish current request */
1088 stm32_hash_finish_req(hdev->req, err);
^^^
Never initialized.
1089
1090 return IRQ_HANDLED;
1091 }
regards,
dan carpenter
Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap
On Fri, Jul 28, 2017 at 09:59:41AM +0530, Suniel Mahesh wrote:
> On Friday 28 July 2017 01:18 AM, Dan Carpenter wrote:
> > On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
> >> + new_drvdata->cc_base = devm_ioremap_resource(_dev->dev,
> >> + req_mem_cc_regs);
> >> + if (IS_ERR(new_drvdata->cc_base)) {
> >> + rc = PTR_ERR(new_drvdata->cc_base);
> >>goto init_cc_res_err;
> >
> > (This code was in the original and not introduced by the patch.)
>
> Hi Dan, the above lines of code were not in the original except
> "goto init_cc_res_err;" which was doing the error handling at different
> places.
>
Yes, yes. I wasn't commenting on the patch just the existing code.
The patch is fine.
regards,
dan carpenter
Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap
On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
> + new_drvdata->cc_base = devm_ioremap_resource(_dev->dev,
> + req_mem_cc_regs);
> + if (IS_ERR(new_drvdata->cc_base)) {
> + rc = PTR_ERR(new_drvdata->cc_base);
> goto init_cc_res_err;
(This code was in the original and not introduced by the patch.)
Ideally, the goto name should say what the goto does. In this case it
does everything. Unfortunately trying to do everything is very
complicated so obviously the error handling is going to be full of bugs.
The first thing the error handling does is:
ssi_aead_free(new_drvdata);
But this function assumes that if new_drvdata->aead_handle is non-NULL
then that means we have called:
INIT_LIST_HEAD(_handle->aead_list);
That assumption is false if the aead_handle->sram_workspace_addr
allocation fails. It can't actually fail in the current code... So
that's good, I suppose. Reviewing this code is really hard, because I
have to jump back and forth through several functions in different
files.
Moving on two the second error handling function:
ssi_hash_free(new_drvdata);
This one has basically the same assumption that if ->hash_handle is
allocated that means we called:
INIT_LIST_HEAD(_handle->hash_list);
That assumption is not true if ssi_hash_init_sram_digest_consts(drvdata);
fails. That function can fail in real life. Except the the error
handling in ssi_hash_alloc() sets ->hash_handle to NULL. So the bug is
just a leak and not a crashing bug.
I've reviewed the first two lines of the error handling just to give a
feel for how complicated "do everything" style error handling is to
review.
The better way to do error handling is:
1) Only free things which have been allocated.
2) The unwind code should mirror the wind up code.
3) Every allocation function should have a free function.
4) Label names should tell you what the goto does.
5) Use direct returns and literals where possible.
6) Generally it's better to keep the error path and the success path
separate.
7) Do error handling as opposed to success handling.
one = alloc();
if (!one)
return -ENOMEM;
if (foo) {
two = alloc();
if (!two) {
ret = -ENOMEM;
goto free_one;
}
}
three = alloc();
if (!three) {
ret = -ENOMEM;
goto free_two;
}
...
return 0;
free_two:
if (foo)
free(two);
free_one:
free(one);
return ret;
This style of error handling is easier to review. You only need to
remember the most recent thing that you have allocated. You can tell
from the goto that it frees it so you don't have to scroll to the
bottom of the function or jump to a different file.
regards,
dan carpenter
[bug report] crypto: chcr - Select device in Round Robin fashion
Hello Harsh Jain,
The patch 14c19b178a01: "crypto: chcr - Select device in Round Robin
fashion" from Jun 15, 2017, leads to the following static checker
warning:
drivers/crypto/chelsio/chcr_core.c:163 chcr_uld_add()
warn: overwrite may leak 'u_ctx'
drivers/crypto/chelsio/chcr_core.c
152 static void *chcr_uld_add(const struct cxgb4_lld_info *lld)
153 {
154 struct uld_ctx *u_ctx;
155
156 /* Create the device and add it in the device list */
157 u_ctx = kzalloc(sizeof(*u_ctx), GFP_KERNEL);
158 if (!u_ctx) {
159 u_ctx = ERR_PTR(-ENOMEM);
160 goto out;
161 }
162 if (!(lld->ulp_crypto & ULP_CRYPTO_LOOKASIDE)) {
Sure, we could move this check before the allocation, to prevent the
leak but is -ENOMEM really the right error code? It feels like -EINVAL
with a WARN_ON_ONCE() message would be better but I don't really
understand this code.
163 u_ctx = ERR_PTR(-ENOMEM);
164 goto out;
165 }
166 u_ctx->lldi = *lld;
167 out:
168 return u_ctx;
169 }
regards,
dan carpenter
[bug report] crypto: atmel-ecc - introduce Microchip / Atmel ECC driver
Hello Tudor-Dan Ambarus,
The patch 11105693fa05: "crypto: atmel-ecc - introduce Microchip /
Atmel ECC driver" from Jul 5, 2017, leads to the following static
checker warning:
drivers/crypto/atmel-ecc.c:281 atmel_ecdh_done()
warn: assigning (-22) to unsigned variable 'status'
drivers/crypto/atmel-ecc.c
265 static void atmel_ecdh_done(struct atmel_ecc_work_data *work_data, void
*areq,
266 u8 status)
267 {
268 struct kpp_request *req = areq;
269 struct atmel_ecdh_ctx *ctx = work_data->ctx;
270 struct atmel_ecc_cmd *cmd = _data->cmd;
271 size_t copied;
272 size_t n_sz = ctx->n_sz;
273
274 if (status)
275 goto free_work_data;
276
277 /* copy the shared secret */
278 copied = sg_copy_from_buffer(req->dst, 1,
>data[RSP_DATA_IDX],
279 n_sz);
280 if (copied != n_sz)
281 status = -EINVAL;
status is a u8.
282
283 /* fall through */
284 free_work_data:
285 kzfree(work_data);
286 kpp_request_complete(req, status);
287 }
regards,
dan carpenter
[bug report] crypto: inside-secure - add SafeXcel EIP197 crypto engine driver
Hello Antoine Tenart,
The patch 1b44c5a60c13: "crypto: inside-secure - add SafeXcel EIP197
crypto engine driver" from May 24, 2017, leads to the following
static checker warning:
drivers/crypto/inside-secure/safexcel_hash.c:890
safexcel_hmac_sha1_setkey()
error: buffer overflow 'ctx->ipad' 5 <= 7
drivers/crypto/inside-secure/safexcel_hash.c
875 static int safexcel_hmac_sha1_setkey(struct crypto_ahash *tfm, const u8
*key,
876 unsigned int keylen)
877 {
878 struct safexcel_ahash_ctx *ctx =
crypto_tfm_ctx(crypto_ahash_tfm(tfm));
879 struct safexcel_ahash_export_state istate, ostate;
880 int ret, i;
881
882 ret = safexcel_hmac_setkey("safexcel-sha1", key, keylen,
, );
883 if (ret)
884 return ret;
885
886 memcpy(ctx->ipad, , SHA1_DIGEST_SIZE);
^
887 memcpy(ctx->opad, , SHA1_DIGEST_SIZE);
^
These are SHA1_DIGEST_SIZE (20).
888
889 for (i = 0; i < ARRAY_SIZE(istate.state); i++) {
This is SHA256_DIGEST_SIZE (32) so it's bigger.
890 if (ctx->ipad[i] != le32_to_cpu(istate.state[i]) ||
891 ctx->opad[i] != le32_to_cpu(ostate.state[i])) {
892 ctx->base.needs_inv = true;
893 break;
894 }
895 }
896
897 return 0;
898 }
regards,
dan carpenter
Re: [PATCH v2 1/7] staging: ccree: fix hash import/export
On Thu, Jun 22, 2017 at 04:36:55PM +0300, Gilad Ben-Yossef wrote:
> static int ssi_ahash_export(struct ahash_request *req, void *out)
> {
> struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
> struct ssi_hash_ctx *ctx = crypto_ahash_ctx(ahash);
> + struct device *dev = >drvdata->plat_dev->dev;
> + struct ahash_req_ctx *state = ahash_request_ctx(req);
> + u8 *curr_buff = state->buff_index ? state->buff1 : state->buff0;
> + u32 curr_buff_cnt = state->buff_index ? state->buff1_cnt :
> + state->buff0_cnt;
> + const u32 tmp = CC_EXPORT_MAGIC;
> +
> + CHECK_AND_RETURN_UPON_FIPS_ERROR();
>
> - return ssi_hash_export(ctx, out);
> + memcpy(out, , sizeof(u32));
> + out += sizeof(u32);
> +
> + dma_sync_single_for_cpu(dev, state->digest_buff_dma_addr,
> + ctx->inter_digestsize, DMA_BIDIRECTIONAL);
> + memcpy(out, state->digest_buff, ctx->inter_digestsize);
> + out += ctx->inter_digestsize;
> +
> + if (state->digest_bytes_len_dma_addr) {
> + dma_sync_single_for_cpu(dev, state->digest_bytes_len_dma_addr,
> + HASH_LEN_SIZE, DMA_BIDIRECTIONAL);
> + memcpy(out, state->digest_bytes_len, HASH_LEN_SIZE);
> + }
> + out += HASH_LEN_SIZE;
I'm sorry to ask this question now instead of on my first review. I was
waiting for my other computer to get ready so I could look up how this
is called. But now that I've looked at it, I'm still not sure how this
is called.
Anyway, my question is, is out zeroed memory? Should we have something
like:
} else {
memset(out, 0, HASH_LEN_SIZE);
}
out += HASH_LEN_SIZE;
The ->import() function has a similar snippet.
regards,
dan carpenter
Re: [PATCH 6/7] staging: ccree: add DT bus coherency detection
On Thu, Jun 22, 2017 at 10:07:52AM +0300, Gilad Ben-Yossef wrote:
> The ccree driver has build time configurable support
> to work on top of coherent (e.g. ACP) vs. none coherent bus
> connections. Turn it to run-time configurable option
> based on device tree.
>
> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
> ---
> drivers/staging/ccree/ssi_buffer_mgr.c | 37
> ++
> drivers/staging/ccree/ssi_config.h | 20 --
> drivers/staging/ccree/ssi_driver.c | 14 +
> drivers/staging/ccree/ssi_driver.h | 3 +++
> 4 files changed, 33 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c
> b/drivers/staging/ccree/ssi_buffer_mgr.c
> index 88ebda8..75810dc 100644
> --- a/drivers/staging/ccree/ssi_buffer_mgr.c
> +++ b/drivers/staging/ccree/ssi_buffer_mgr.c
> @@ -627,6 +627,9 @@ void ssi_buffer_mgr_unmap_aead_request(
> struct aead_req_ctx *areq_ctx = aead_request_ctx(req);
> unsigned int hw_iv_size = areq_ctx->hw_iv_size;
> struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> + struct ssi_drvdata *drvdata =
> + (struct ssi_drvdata *)dev_get_drvdata(dev);
The cast is not needed.
> +
Don't put a blank in the middle of the declaration block.
> u32 dummy;
> bool chained;
> u32 size_to_unmap = 0;
[ snip ]
> @@ -981,20 +983,22 @@ static inline int ssi_buffer_mgr_prepare_aead_data_mlli(
>* MAC verification upon request completion
>*/
> if (direct == DRV_CRYPTO_DIRECTION_DECRYPT) {
> -#if !DX_HAS_ACP
> - /* In ACP platform we already copying ICV
> - * for any INPLACE-DECRYPT operation, hence
> + if (!drvdata->coherent) {
> + /* In coherent platforms (e.g. ACP)
> + * already copying ICV for any
> + * INPLACE-DECRYPT operation, hence
>* we must neglect this code.
>*/
> - u32 size_to_skip = req->assoclen;
> - if (areq_ctx->is_gcm4543) {
> - size_to_skip += crypto_aead_ivsize(tfm);
> + u32 skip = req->assoclen;
> +
> + if (areq_ctx->is_gcm4543)
> + skip += crypto_aead_ivsize(tfm);
> +
> + ssi_buffer_mgr_copy_scatterlist_portion(
> +areq_ctx->backup_mac, req->src,
> +(skip + req->cryptlen - areq_ctx->req_authsize),
> +skip + req->cryptlen, SSI_SG_TO_BUF);
Indenting is messed up.
> }
> - ssi_buffer_mgr_copy_scatterlist_portion(
> - areq_ctx->backup_mac, req->src,
> - size_to_skip+ req->cryptlen -
> areq_ctx->req_authsize,
> - size_to_skip+ req->cryptlen,
> SSI_SG_TO_BUF);
> -#endif
> areq_ctx->icv_virt_addr = areq_ctx->backup_mac;
> } else {
> areq_ctx->icv_virt_addr = areq_ctx->mac_buf;
[ snip ]
> @@ -533,7 +539,7 @@ int cc_clk_on(struct ssi_drvdata *drvdata)
> struct clk *clk = drvdata->clk;
>
> if (IS_ERR(clk))
> - /* No all devices have a clock associated with CCREE */
> + /* Not all devices have a clock associated with CCREE */
This is not related. Do unrelated things in different patches. This
typo was introduced in an earlier patch. There are a couple ways in git
to edit previous patches.
> goto out;
>
> rc = clk_prepare_enable(clk);
regards,
dan carpenter
Re: [PATCH 5/7] staging: ccree: add clock management support
On Thu, Jun 22, 2017 at 10:07:51AM +0300, Gilad Ben-Yossef wrote:
> +int cc_clk_on(struct ssi_drvdata *drvdata)
> +{
> + int rc = 0;
> + struct clk *clk = drvdata->clk;
> +
> + if (IS_ERR(clk))
> + /* No all devices have a clock associated with CCREE */
> + goto out;
Ugh... I hate this. The "goto out;" here is a waste of time
do-nothing-goto that returns diretly. It's equivalent to "return 0;".
Is that intended? Even with the comment, it's not clear...
People think do nothing gotos are a great idea but from reviewing tons
and tons of real life errors, I can assure you that in real life (as
opposed to theory) they don't prevent any future bugs and only introduce
"forgot to set the error code" bugs.
The indenting is messed up and multi-line indents get curly braces.
> +
> + rc = clk_prepare_enable(clk);
> + if (rc) {
> + SSI_LOG_ERR("error enabling clock\n");
> + clk_disable_unprepare(clk);
Don't unprepare something that hasn't been prepared.
> + }
> +
> +out:
> + return rc;
> +}
int cc_clk_on(struct ssi_drvdata *drvdata)
{
struct clk *clk = drvdata->clk;
int rc;
if (IS_ERR(clk)) {
/* Not all devices have a clock associated with CCREE */
return 0;
}
rc = clk_prepare_enable(clk);
if (rc)
return rc;
return 0;
}
regards,
dan carpenter
Re: [PATCH 0/7] staging: ccree: bug fixes and TODO items for 4.13
On Thu, Jun 22, 2017 at 10:14:08AM +0300, Gilad Ben-Yossef wrote: > On Thu, Jun 22, 2017 at 10:07 AM, Gilad Ben-Yossef <gi...@benyossef.com> > wrote: > > An assortment of bug fixes and staging TODO items. > > Highlights includes the driver passing crypto testmgr boot tests > > and support of multiple HW revs. without build time changes. > > > > Gilad Ben-Yossef (7): > > staging: ccree: fix hash import/export > > staging: ccree: register setkey for none hash macs > > staging: ccree: add support for older HW revisions > > staging: ccree: remove unused function > > staging: ccree: add clock management support > > staging: ccree: add DT bus coherency detection > > staging: ccree: use signal safe completion wait > > One additional note: the patch set is on top of the current staging-next which > is d06838de4a015c7c4844ad3fcf63ad5e2c17b234 so it will conflict with > the coding style clean up patches from Jhin-Ming if you take them. > > If you wish me to merge this patch set on top those just let me know. > Yes. Those are fine and will be merged most likely. It's strictly first in, first out. regards, dan carpenter
Re: [PATCH] staging: ccree: fix coding style error
On Tue, Jun 20, 2017 at 10:51:58PM +0800, Jhih-Ming Huang wrote: > > Hi, > > This patch fix all coding style error in driver/staging/ccree/ssi_aead.c. Much better. Thanks! regards, dan carpenter
Re: [PATCH 05/11] Fix ERROR: space prohibited after that open parenthesis '('
On Tue, Jun 20, 2017 at 01:21:46PM +0800, Jhih-Ming Huang wrote:
> From: Jhih-Ming Hunag <fbihjme...@gmail.com>
>
> Fixed "ERROR: space prohibited after that open parenthesis '('".
>
> Signed-off-by: Jhih-Ming Hunag <fbihjme...@gmail.com>
> ---
> drivers/staging/ccree/ssi_aead.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_aead.c
> b/drivers/staging/ccree/ssi_aead.c
> index 6bcab5a..5166874 100644
> --- a/drivers/staging/ccree/ssi_aead.c
> +++ b/drivers/staging/ccree/ssi_aead.c
> @@ -1375,10 +1375,10 @@ static int validate_data_size(struct ssi_aead_ctx
> *ctx,
> static unsigned int format_ccm_a0(u8 *pA0Buff, u32 headerSize)
> {
> unsigned int len = 0;
> - if ( headerSize == 0 ) {
> + if (headerSize == 0 ) {
Remove the other space as well. I looked ahead in the series so I see
that you do it later, but it should be done here.
regards,
dan carpenter
Re: [PATCH 02/11] Fix ERROR: spaces required around that
On Tue, Jun 20, 2017 at 01:20:59PM +0800, Jhih-Ming Huang wrote: > From: Jhih-Ming Hunag <fbihjme...@gmail.com> > > Fixed 'ERROR: spaces required around that' > You're breaking the patches up in a bad way. This one should be combined with the previous patch. regards, dan carpenter
Re: [PATCH 01/11] Fix coding style of driver/staging/ccree/ssi_aead.c ERROR: space required after that
Subject is wrong. It should be: [PATCH 1/11] Staging: ccree: add spaces blah blah blah On Tue, Jun 20, 2017 at 01:19:44PM +0800, Jhih-Ming Huang wrote: > From: Jhih-Ming Hunag <fbihjme...@gmail.com> > No need. > In this series patches, I fix all of the coding style error in > driver/staging/ccree/ssi_aead.c from 54 errors to 0 error. You could put this into the cover letter. When we put this into the final git log we don't see the series only individual patches. > > The first patch fixed 'ERROR: space required after that'. > This patch fixes ... regards, dan carpenter
[PATCH] crypto: cavium/nitrox - dma_mapping_error() returns bool
We want to return negative error codes here, but we're accidentally
propogating the "true" return from dma_mapping_error().
Fixes: 14fa93cdcd9b ("crypto: cavium - Add support for CNN55XX adapters.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
b/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
index b6bd2a870028..4bb4377c5ac0 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
@@ -199,9 +199,10 @@ static int dma_map_inbufs(struct nitrox_softreq *sr,
sr->in.sglist = glist;
/* map IV */
dma = dma_map_single(dev, >iv, req->ivsize, DMA_BIDIRECTIONAL);
- ret = dma_mapping_error(dev, dma);
- if (ret)
+ if (dma_mapping_error(dev, dma)) {
+ ret = -EINVAL;
goto iv_map_err;
+ }
sr->in.dir = (req->src == req->dst) ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE;
/* map src entries */
@@ -268,16 +269,18 @@ static int dma_map_outbufs(struct nitrox_softreq *sr,
/* map ORH */
sr->resp.orh_dma = dma_map_single(dev, >resp.orh, ORH_HLEN,
sr->out.dir);
- ret = dma_mapping_error(dev, sr->resp.orh_dma);
- if (ret)
+ if (dma_mapping_error(dev, sr->resp.orh_dma)) {
+ ret = -EINVAL;
goto orh_map_err;
+ }
/* map completion */
sr->resp.completion_dma = dma_map_single(dev, >resp.completion,
COMP_HLEN, sr->out.dir);
- ret = dma_mapping_error(dev, sr->resp.completion_dma);
- if (ret)
+ if (dma_mapping_error(dev, sr->resp.completion_dma)) {
+ ret = -EINVAL;
goto compl_map_err;
+ }
sr->inplace = (req->src == req->dst) ? true : false;
/* out place */
[PATCH v2] X.509: Fix error code in x509_cert_parse()
We forgot to set the error code on this path so it could result in
returning NULL which leads to a NULL dereference.
Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the
akcipher api")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
v2: Style change
Sorry for the delay, I'm been out of office.
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index c80765b211cf..dd03fead1ca3 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -102,6 +102,7 @@ struct x509_certificate *x509_cert_parse(const void *data,
size_t datalen)
}
}
+ ret = -ENOMEM;
cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL);
if (!cert->pub->key)
goto error_decode;
[PATCH] X.509: Fix error code in x509_cert_parse()
We forgot to set the error code on this path so it could result in
returning NULL which leads to a NULL dereference.
Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the
akcipher api")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index c80765b211cf..1f69e948fb34 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -103,8 +103,10 @@ struct x509_certificate *x509_cert_parse(const void *data,
size_t datalen)
}
cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL);
- if (!cert->pub->key)
+ if (!cert->pub->key) {
+ ret = -ENOMEM;
goto error_decode;
+ }
cert->pub->keylen = ctx->key_size;
[PATCH] crypto: glue_helper - Delete some dead code
We checked (nbytes < bsize) inside the loops so it's not possible to hit the "goto done;" here. This code is cut and paste from other slightly different loops where we don't have the check inside the loop. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/arch/x86/crypto/glue_helper.c b/arch/x86/crypto/glue_helper.c index 24ac9fad832d..d61e57960fe0 100644 --- a/arch/x86/crypto/glue_helper.c +++ b/arch/x86/crypto/glue_helper.c @@ -176,9 +176,6 @@ __glue_cbc_decrypt_128bit(const struct common_glue_ctx *gctx, src -= 1; dst -= 1; } while (nbytes >= func_bytes); - - if (nbytes < bsize) - goto done; } }
[PATCH] crypto: sha512-mb - add some missing unlock on error
We recently added some new locking but missed the unlocks on these
error paths in sha512_ctx_mgr_submit().
Fixes: c459bd7beda0 ("crypto: sha512-mb - Protect sha512 mb ctx mgr access")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/arch/x86/crypto/sha512-mb/sha512_mb.c
b/arch/x86/crypto/sha512-mb/sha512_mb.c
index 2dd3674b5a1e..458409b7568d 100644
--- a/arch/x86/crypto/sha512-mb/sha512_mb.c
+++ b/arch/x86/crypto/sha512-mb/sha512_mb.c
@@ -269,19 +269,19 @@ static struct sha512_hash_ctx
* LAST
*/
ctx->error = HASH_CTX_ERROR_INVALID_FLAGS;
- return ctx;
+ goto unlock;
}
if (ctx->status & HASH_CTX_STS_PROCESSING) {
/* Cannot submit to a currently processing job. */
ctx->error = HASH_CTX_ERROR_ALREADY_PROCESSING;
- return ctx;
+ goto unlock;
}
if ((ctx->status & HASH_CTX_STS_COMPLETE) && !(flags & HASH_FIRST)) {
/* Cannot update a finished job. */
ctx->error = HASH_CTX_ERROR_ALREADY_COMPLETED;
- return ctx;
+ goto unlock;
}
@@ -363,6 +363,7 @@ static struct sha512_hash_ctx
}
ctx = sha512_ctx_mgr_resubmit(mgr, ctx);
+unlock:
spin_unlock_irqrestore(>work_lock, irqflags);
return ctx;
}
Re: [PATCH 2/2] crypto: chcr - Fix error checking
On Thu, Apr 13, 2017 at 08:37:50PM +0530, Harsh Jain wrote:
> On Thu, Apr 13, 2017 at 8:20 PM, Christophe JAILLET
> <christophe.jail...@wanadoo.fr> wrote:
> > Le 13/04/2017 à 16:04, Dan Carpenter a écrit :
> >>
> >> On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
> >>>
> >>> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
> >>> error pointer when we 'goto out'.
> >>> So checking for NULL here is not enough because it is likely that
> >>> 'chcr_free_shash' will crash if we pass an error pointer.
> >>>
> >>> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> >>> ---
> >>> Another solution, amybe safer, would be to instrument 'chcr_free_shash'
> >>> or
> >>> 'crypto_free_shash' to accept an error pointer and return immediatelly in
> >>> such a case.
> >>> ---
> >>> drivers/crypto/chelsio/chcr_algo.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/crypto/chelsio/chcr_algo.c
> >>> b/drivers/crypto/chelsio/chcr_algo.c
> >>> index f19590ac8775..41750b97f43c 100644
> >>> --- a/drivers/crypto/chelsio/chcr_algo.c
> >>> +++ b/drivers/crypto/chelsio/chcr_algo.c
> >>> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead
> >>> *authenc, const u8 *key,
> >>> }
> >>> out:
> >>> aeadctx->enckey_len = 0;
> >>> - if (base_hash)
> >>> + if (!IS_ERR_OR_NULL(base_hash))
> >>> chcr_free_shash(base_hash);
> >>
> >> Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
> >> ever be NULL.
> >>
> >> regards,
> >> dan carpenter
> >
> > Hi Dan,
> >
> > I will update the first patch as you proposed in order to:
> >- teach 'chcr_alloc_shash' not to return NULL
> >- initialize 'base_hash' with ERR_PTR(-EINVAL)
> >- update the above test to !IS_ERR.
> > The 2 patches will be merged in only 1.
> >
> > Thanks for your suggestions.
>
> Thanks for pointing the error. or You can simply return instead of
> goto. Just like that.
>
> 1.3 @@ -2455,7 +2455,8 @@ static int chcr_authenc_setkey(struct cr
> 1.4 base_hash = chcr_alloc_shash(max_authsize);
> 1.5 if (IS_ERR(base_hash)) {
> 1.6 pr_err("chcr : Base driver cannot be loaded\n");
> 1.7 - goto out;
> 1.8 + aeadctx->enckey_len = 0;
> 1.9 + return -EINVAL;
Don't do that. There should be a goto.
regards,
dan carpenter
Re: [PATCH 1/2] crypto: chcr - Improve error checking
On Thu, Apr 13, 2017 at 02:14:19PM +0200, Christophe JAILLET wrote:
> 'chcr_alloc_shash()' can return NULL. Here it is not possible because this
> code is reached only if 'get_alg_config()' a few lines above has succeeded.
> So we are garanteed that the value of 'max_authsize' is a correct
> parameter.
> Anyway, this is harmless to add a check for NULL.
>
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
> drivers/crypto/chelsio/chcr_algo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c
> b/drivers/crypto/chelsio/chcr_algo.c
> index 41bc7f4f58cd..f19590ac8775 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2294,7 +2294,7 @@ static int chcr_authenc_setkey(struct crypto_aead
> *authenc, const u8 *key,
> aeadctx->enckey_len << 3);
>
> base_hash = chcr_alloc_shash(max_authsize);
> - if (IS_ERR(base_hash)) {
> + if (IS_ERR_OR_NULL(base_hash)) {
> pr_err("chcr : Base driver cannot be loaded\n");
> goto out;
Ugh... When you mix NULL and error pointers, it should be because NULL
is a valid return. We should change chcr_alloc_shash() to return
ERR_PTR(-EINVAL) instead of NULL.
Also the "goto out;" is buggy, of course. The problem with magical free
everything style error handling is that "base_hash" wasn't allocated so
this will oops for both NULL and error pointers.
regards,
dan carpenter
Re: [PATCH 2/2] crypto: chcr - Fix error checking
On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote: > If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an > error pointer when we 'goto out'. > So checking for NULL here is not enough because it is likely that > 'chcr_free_shash' will crash if we pass an error pointer. > > Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr> > --- > Another solution, amybe safer, would be to instrument 'chcr_free_shash' or > 'crypto_free_shash' to accept an error pointer and return immediatelly in > such a case. > --- > drivers/crypto/chelsio/chcr_algo.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/chelsio/chcr_algo.c > b/drivers/crypto/chelsio/chcr_algo.c > index f19590ac8775..41750b97f43c 100644 > --- a/drivers/crypto/chelsio/chcr_algo.c > +++ b/drivers/crypto/chelsio/chcr_algo.c > @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead > *authenc, const u8 *key, > } > out: > aeadctx->enckey_len = 0; > - if (base_hash) > + if (!IS_ERR_OR_NULL(base_hash)) > chcr_free_shash(base_hash); Ah... Ok. Fine, but redo the first patch anyway because it shouldn't ever be NULL. regards, dan carpenter
[bug report] padata: simplify serialization mechanism
Hello Steffen Klassert,
The patch 5f1a8c1bc724: "padata: simplify serialization mechanism"
from Jul 7, 2010, leads to the following static checker warning:
kernel/padata.c:243 padata_reorder()
warn: 'padata' is an error pointer or valid
kernel/padata.c
231 if (!spin_trylock_bh(>lock))
232 return;
233
234 while (1) {
235 padata = padata_get_next(pd);
236
237 /*
238 * All reorder queues are empty, or the next object
that needs
239 * serialization is parallel processed by another cpu
and is
240 * still on it's way to the cpu's reorder queue,
nothing to
241 * do for now.
242 */
243 if (!padata || PTR_ERR(padata) == -EINPROGRESS)
^^
The comments still say that we sometimes return NULL but we removed that
in 2010.
244 break;
245
246 /*
247 * This cpu has to do the parallel processing of the
next
248 * object. It's waiting in the cpu's parallelization
queue,
249 * so exit immediately.
250 */
251 if (PTR_ERR(padata) == -ENODATA) {
252 del_timer(>timer);
253 spin_unlock_bh(>lock);
254 return;
255 }
regards,
dan carpenter
Re: [PATCH] crypto: zip - Memory corruption in zip_clear_stats()
On Sat, Mar 18, 2017 at 11:24:34AM +0100, walter harms wrote:
>
>
> Am 17.03.2017 21:46, schrieb Dan Carpenter:
> > There is a typo here. It should be "stats" instead of "state". The
> > impact is that we clear 224 bytes instead of 80 and we zero out memory
> > that we shouldn't.
> >
> > Fixes: 09ae5d37e093 ("crypto: zip - Add Compression/Decompression
> > statistics")
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> >
> > diff --git a/drivers/crypto/cavium/zip/zip_main.c
> > b/drivers/crypto/cavium/zip/zip_main.c
> > index 0951e20b395b..6ff13d80d82e 100644
> > --- a/drivers/crypto/cavium/zip/zip_main.c
> > +++ b/drivers/crypto/cavium/zip/zip_main.c
> > @@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void
> > *unused)
> > for (index = 0; index < MAX_ZIP_DEVICES; index++) {
> > if (zip_dev[index]) {
> > memset(_dev[index]->stats, 0,
> > - sizeof(struct zip_state));
> > + sizeof(struct zip_stats));
>
>
> as future FIXME some show find a name that differ in more than just the last
> char.
> NTL maybe
> sizeof(zip_dev[index]->stats)
> can be used here ?
That's sort of unweildy. I don't fear that change because I'm confident
I would catch it with static analysis.
regards,
dan carpenter
[PATCH] crypto: zip - Memory corruption in zip_clear_stats()
There is a typo here. It should be "stats" instead of "state". The
impact is that we clear 224 bytes instead of 80 and we zero out memory
that we shouldn't.
Fixes: 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/cavium/zip/zip_main.c
b/drivers/crypto/cavium/zip/zip_main.c
index 0951e20b395b..6ff13d80d82e 100644
--- a/drivers/crypto/cavium/zip/zip_main.c
+++ b/drivers/crypto/cavium/zip/zip_main.c
@@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void *unused)
for (index = 0; index < MAX_ZIP_DEVICES; index++) {
if (zip_dev[index]) {
memset(_dev[index]->stats, 0,
- sizeof(struct zip_state));
+ sizeof(struct zip_stats));
seq_printf(s, "Cleared stats for zip %d\n", index);
}
}
[bug report] crypto: cavium - Add the Virtual Function driver for CPT
Hello George Cherian,
This is a semi-automatic email about new static checker warnings.
The patch c694b233295b: "crypto: cavium - Add the Virtual Function
driver for CPT" from Feb 7, 2017, leads to the following Smatch
complaint:
drivers/crypto/cavium/cpt/cptvf_main.c:899 cptvf_remove()
error: we previously assumed 'cptvf' could be null (see line 895)
drivers/crypto/cavium/cpt/cptvf_main.c
894
895 if (!cptvf)
^
Check for NULL.
896 dev_err(>dev, "Invalid CPT-VF device\n");
897
898 /* Convey DOWN to PF */
899 if (cptvf_send_vf_down(cptvf)) {
^
Oops inside function.
900 dev_err(>dev, "PF not responding to DOWN msg");
901 } else {
regards,
dan carpenter
[bug report] crypto: cavium - Add the Virtual Function driver for CPT
Hello George Cherian,
This is a semi-automatic email about new static checker warnings.
The patch c694b233295b: "crypto: cavium - Add the Virtual Function
driver for CPT" from Feb 7, 2017, leads to the following Smatch
complaint:
drivers/crypto/cavium/cpt/cptvf_reqmanager.c:333 do_post_process()
warn: variable dereferenced before check 'cptvf' (see line 331)
drivers/crypto/cavium/cpt/cptvf_reqmanager.c
330 {
331 struct pci_dev *pdev = cptvf->pdev;
^^^
Dereference.
332
333 if (!info || !cptvf) {
^
Check is too late.
334 dev_err(>dev, "Input params are incorrect for
post processing\n");
335 return;
regards,
dan carpenter
[bug report] crypto: cavium - Add Support for Octeon-tx CPT Engine
Hello George Cherian,
The patch 9e2c7d99941d: "crypto: cavium - Add Support for Octeon-tx
CPT Engine" from Feb 7, 2017, leads to the following static checker
warning:
drivers/crypto/cavium/cpt/cptpf_mbox.c:70 cpt_bind_vq_to_grp()
warn: signedness bug returning '(-22)'
drivers/crypto/cavium/cpt/cptpf_mbox.c
62 static u8 cpt_bind_vq_to_grp(struct cpt_device *cpt, u8 q, u8 grp)
^^
63 {
64 struct microcode *mcode = cpt->mcode;
65 union cptx_pf_qx_ctl pf_qx_ctl;
66 struct device *dev = >pdev->dev;
67
68 if (q >= CPT_MAX_VF_NUM) {
69 dev_err(dev, "Queues are more than cores in the group");
70 return -EINVAL;
^^^
71 }
72 if (grp >= CPT_MAX_CORE_GROUPS) {
73 dev_err(dev, "Request group is more than possible
groups");
74 return -EINVAL;
^^^
75 }
76 if (grp >= cpt->next_mc_idx) {
77 dev_err(dev, "Request group is higher than available
functional groups");
78 return -EINVAL;
^^^
79 }
80 pf_qx_ctl.u = cpt_read_csr64(cpt->reg_base, CPTX_PF_QX_CTL(0,
q));
81 pf_qx_ctl.s.grp = mcode[grp].group;
82 cpt_write_csr64(cpt->reg_base, CPTX_PF_QX_CTL(0, q),
pf_qx_ctl.u);
83 dev_dbg(dev, "VF %d TYPE %s", q, (mcode[grp].is_ae ? "AE" :
"SE"));
84
85 return mcode[grp].is_ae ? AE_TYPES : SE_TYPES;
86 }
regards,
dan carpenter
[bug report] crypto: brcm - Add Broadcom SPU driver
Hello Rob Rice,
The patch 9d12ba86f818: "crypto: brcm - Add Broadcom SPU driver" from
Feb 3, 2017, leads to the following static checker warning:
drivers/crypto/bcm/cipher.c:2340 ahash_finup()
warn: 'tmpbuf' was already freed.
drivers/crypto/bcm/cipher.c
2316 /* Copy data from req scatterlist to tmp buffer */
2317 gfp = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
2318 CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL :
GFP_ATOMIC;
2319 tmpbuf = kmalloc(req->nbytes, gfp);
2320 if (!tmpbuf) {
2321 ret = -ENOMEM;
2322 goto ahash_finup_exit;
2323 }
2324
2325 if (sg_copy_to_buffer(req->src, nents, tmpbuf,
req->nbytes) !=
2326 req->nbytes) {
2327 ret = -EINVAL;
2328 goto ahash_finup_free;
2329 }
2330
2331 /* Call synchronous update */
2332 ret = crypto_shash_finup(ctx->shash, tmpbuf,
req->nbytes,
2333 req->result);
2334 kfree(tmpbuf);
^
2335 } else {
2336 /* Otherwise call the internal function which uses SPU
hw */
2337 return __ahash_finup(req);
2338 }
2339 ahash_finup_free:
2340 kfree(tmpbuf);
^
I'm only working a 30 minutes per day to keep a hand in. I'm not
sending patches this month.
2341
2342 ahash_finup_exit:
2343 /* Done with hash, can deallocate it now */
2344 crypto_free_shash(ctx->shash->tfm);
2345 kfree(ctx->shash);
2346 return ret;
2347 }
regards,
dan carpenter
[bug report] crypto: atmel-sha - update request queue management to make it more generic
Hello Cyrille Pitchen,
The patch a29af939b24d: "crypto: atmel-sha - update request queue
management to make it more generic" from Jan 26, 2017, leads to the
following static checker warning:
drivers/crypto/atmel-sha.c:673 atmel_sha_xmit_dma()
error: we previously assumed 'in_desc' could be null (see line 670)
drivers/crypto/atmel-sha.c
652
653 dmaengine_slave_config(dd->dma_lch_in.chan,
>dma_lch_in.dma_conf);
654
655 if (length2) {
656 sg_init_table(sg, 2);
657 sg_dma_address([0]) = dma_addr1;
658 sg_dma_len([0]) = length1;
659 sg_dma_address([1]) = dma_addr2;
660 sg_dma_len([1]) = length2;
661 in_desc = dmaengine_prep_slave_sg(dd->dma_lch_in.chan,
sg, 2,
662 DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT |
DMA_CTRL_ACK);
663 } else {
664 sg_init_table(sg, 1);
665 sg_dma_address([0]) = dma_addr1;
666 sg_dma_len([0]) = length1;
667 in_desc = dmaengine_prep_slave_sg(dd->dma_lch_in.chan,
sg, 1,
668 DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT |
DMA_CTRL_ACK);
669 }
670 if (!in_desc)
671 atmel_sha_complete(dd, -EINVAL);
Did you mean return atmel_sha_complete(dd, -EINVAL);??? That patch
change a bunch of returns to just call atmel_sha_complete(). Someone
should probably review it again to make sure there aren't other bugs
as well.
672
673 in_desc->callback = atmel_sha_dma_callback;
^
NULL dereference.
674 in_desc->callback_param = dd;
675
676 atmel_sha_write_ctrl(dd, 1);
677
regards,
dan carpenter
Re: [bug report] crypto: chcr - Add AEAD algos.
Ping?
regards,
dan carpenter
On Thu, Dec 01, 2016 at 11:48:58PM +0300, Dan Carpenter wrote:
> Hello Harsh Jain,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 2debd3325e55: "crypto: chcr - Add AEAD algos." from Nov 29,
> 2016, leads to the following Smatch complaint:
>
> drivers/crypto/chelsio/chcr_algo.c:2460 chcr_aead_op()
> warn: variable dereferenced before check 'ctx' (see line 2457)
>
> drivers/crypto/chelsio/chcr_algo.c:2460 chcr_aead_op()
> warn: variable dereferenced before check 'ctx->dev' (see line 2457)
>
> drivers/crypto/chelsio/chcr_algo.c
> 2456struct chcr_context *ctx = crypto_aead_ctx(tfm);
> 2457struct uld_ctx *u_ctx = ULD_CTX(ctx);
>
> Derefences both.
>
> 2458struct sk_buff *skb;
> 2459
> 2460if (ctx && !ctx->dev) {
>
> Check is too late.
>
> 2461pr_err("chcr : %s : No crypto device.\n",
> __func__);
> 2462return -ENXIO;
>
> regards,
> dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[bug report] crypto: chcr - Add AEAD algos.
Hello Harsh Jain,
This is a semi-automatic email about new static checker warnings.
The patch 2debd3325e55: "crypto: chcr - Add AEAD algos." from Nov 29,
2016, leads to the following Smatch complaint:
drivers/crypto/chelsio/chcr_algo.c:2460 chcr_aead_op()
warn: variable dereferenced before check 'ctx' (see line 2457)
drivers/crypto/chelsio/chcr_algo.c:2460 chcr_aead_op()
warn: variable dereferenced before check 'ctx->dev' (see line 2457)
drivers/crypto/chelsio/chcr_algo.c
2456 struct chcr_context *ctx = crypto_aead_ctx(tfm);
2457 struct uld_ctx *u_ctx = ULD_CTX(ctx);
Derefences both.
2458 struct sk_buff *skb;
2459
2460 if (ctx && !ctx->dev) {
Check is too late.
2461 pr_err("chcr : %s : No crypto device.\n", __func__);
2462 return -ENXIO;
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] s390/crypto: unlock on error in prng_tdes_read()
We added some new locking but forgot to unlock on error.
Fixes: 57127645d79d ("s390/zcrypt: Introduce new SHA-512 based Pseudo Random
Generator.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/arch/s390/crypto/prng.c b/arch/s390/crypto/prng.c
index 9cc050f..1113389 100644
--- a/arch/s390/crypto/prng.c
+++ b/arch/s390/crypto/prng.c
@@ -507,8 +507,10 @@ static ssize_t prng_tdes_read(struct file *file, char
__user *ubuf,
prng_data->prngws.byte_counter += n;
prng_data->prngws.reseed_counter += n;
- if (copy_to_user(ubuf, prng_data->buf, chunk))
- return -EFAULT;
+ if (copy_to_user(ubuf, prng_data->buf, chunk)) {
+ ret = -EFAULT;
+ break;
+ }
nbytes -= chunk;
ret += chunk;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[bug report] crypto: omap-sham - add support functions for sg based data handling
Hello Tero Kristo,
This is a semi-automatic email about new static checker warnings.
The patch f19de1bc67a0: "crypto: omap-sham - add support functions
for sg based data handling" from Sep 19, 2016, leads to the following
Smatch complaint:
drivers/crypto/omap-sham.c:808 omap_sham_prepare_request()
warn: variable dereferenced before check 'req' (see line 801)
drivers/crypto/omap-sham.c
800 {
801 struct omap_sham_reqctx *rctx = ahash_request_ctx(req);
^^^
New dereference inside function.
802 int bs;
803 int ret;
804 int nbytes;
805 bool final = rctx->flags & BIT(FLAGS_FINUP);
806 int xmit_len, hash_later;
807
808 if (!req)
New check is too late.
809 return 0;
810
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[bug report] crypto: ccp - Let a v5 CCP provide the same function as v3
Hello Gary R Hook,
The patch 4b394a232df7: "crypto: ccp - Let a v5 CCP provide the same
function as v3" from Jul 26, 2016, leads to the following static
checker warning:
drivers/crypto/ccp/ccp-dev-v5.c:30 ccp_lsb_alloc()
warn: always true condition '(cmd_q->lsb >= 0) => (0-u32max >= 0)'
drivers/crypto/ccp/ccp-dev-v5.c
24 static u32 ccp_lsb_alloc(struct ccp_cmd_queue *cmd_q, unsigned int
count)
25 {
26 struct ccp_device *ccp;
27 int start;
28
29 /* First look at the map for the queue */
30 if (cmd_q->lsb >= 0) {
^^^
->lsb is a u32 so this is always true.
31 start = (u32)bitmap_find_next_zero_area(cmd_q->lsbmap,
32 LSB_SIZE,
33 0, count, 0);
34 if (start < LSB_SIZE) {
35 bitmap_set(cmd_q->lsbmap, start, count);
36 return start + cmd_q->lsb * LSB_SIZE;
37 }
38 }
39
40 /* No joy; try to get an entry from the shared blocks */
41 ccp = cmd_q->ccp;
42 for (;;) {
43 mutex_lock(>sb_mutex);
44
45 start = (u32)bitmap_find_next_zero_area(ccp->lsbmap,
46 MAX_LSB_CNT *
LSB_SIZE,
47 0,
48 count, 0);
49 if (start <= MAX_LSB_CNT * LSB_SIZE) {
50 bitmap_set(ccp->lsbmap, start, count);
51
52 mutex_unlock(>sb_mutex);
53 return start * LSB_ITEM_SIZE;
54 }
55
56 ccp->sb_avail = 0;
57
58 mutex_unlock(>sb_mutex);
59
60 /* Wait for KSB entries to become available */
61 if (wait_event_interruptible(ccp->sb_queue,
ccp->sb_avail))
62 return 0;
63 }
64 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[bug report] chcr: Support for Chelsio's Crypto Hardware
Hello Hariprasad Shenai,
This is a semi-automatic email about new static checker warnings.
The patch 324429d74127: "chcr: Support for Chelsio's Crypto Hardware"
from Aug 17, 2016, leads to the following Smatch complaint:
drivers/crypto/chelsio/chcr_algo.c:378 write_sg_data_page_desc()
error: we previously assumed 'sg' could be null (see line 376)
drivers/crypto/chelsio/chcr_algo.c
375 while (count > 0) {
376 if (sg && (!(sg->length)))
^^
Check.
377 break;
378 spage = sg_page(sg);
^^^
Unchecked dereference inside function.
379 get_page(spage);
380 page_len = min(sg->length, count);
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: nx - off by one bug in nx_of_update_msc()
The props->ap[] array is defined like this:
struct alg_props ap[NX_MAX_FC][NX_MAX_MODE][3];
So we can see that if msc->fc and msc->mode are == to NX_MAX_FC or
NX_MAX_MODE then we're off by one.
Fixes: ae0222b7289d ('powerpc/crypto: nx driver code supporting nx encryption')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/nx/nx.c b/drivers/crypto/nx/nx.c
index 0794f1c..42f0f22 100644
--- a/drivers/crypto/nx/nx.c
+++ b/drivers/crypto/nx/nx.c
@@ -392,7 +392,7 @@ static void nx_of_update_msc(struct device *dev,
((bytes_so_far + sizeof(struct msc_triplet)) <= lenp) &&
i < msc->triplets;
i++) {
- if (msc->fc > NX_MAX_FC || msc->mode > NX_MAX_MODE) {
+ if (msc->fc >= NX_MAX_FC || msc->mode >= NX_MAX_MODE) {
dev_err(dev, "unknown function code/mode "
"combo: %d/%d (ignored)\n", msc->fc,
msc->mode);
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] crypto: sha256-mb - cleanup a || vs | typo
On Thu, Jun 30, 2016 at 01:42:19PM -0700, Tim Chen wrote:
> On Wed, 2016-06-29 at 10:05 -0700, H. Peter Anvin wrote:
> > On 06/29/16 07:42, Dan Carpenter wrote:
> > >
> > > >
> > > > >
> > > > > and | behave basically the same here but || is intended. It causes a
> > > static checker warning to mix up bitwise and logical operations.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> > >
> > > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > index c9d5dcc..4ec895a 100644
> > > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx
> > > *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> > > * Or if the user's buffer contains less than a whole block,
> > > * append as much as possible to the extra block.
> > > */
> > > - if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
> > > + if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) {
> > > /* Compute how many bytes to copy from user buffer into
> > > * extra block
> > > */
> > >
> > As far as I know the | was an intentional optimization, so you may way
> > to look at the generated code.
> >
> > -hpa
> >
>
> Yes, this is an intentional optimization. Is there any scenario where things
> may
> break with the compiler?
No. I'm going to remove the warning from the static checker like I said
earlier. It should only complain for && vs & typos, || vs | is
harmless.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] crypto: sha256-mb - cleanup a || vs | typo
The difference between | and || is that || has ordering constraints. It's from the C standard, and not the compiler version. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] crypto: sha256-mb - cleanup a || vs | typo
On Wed, Jun 29, 2016 at 10:05:53AM -0700, H. Peter Anvin wrote:
> On 06/29/16 07:42, Dan Carpenter wrote:
> > || and | behave basically the same here but || is intended. It causes a
> > static checker warning to mix up bitwise and logical operations.
> >
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> >
> > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > index c9d5dcc..4ec895a 100644
> > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx
> > *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> > * Or if the user's buffer contains less than a whole block,
> > * append as much as possible to the extra block.
> > */
> > - if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
> > + if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) {
> > /* Compute how many bytes to copy from user buffer into
> > * extra block
> > */
> >
>
> As far as I know the | was an intentional optimization, so you may way
> to look at the generated code.
I know how the rules work. I just thought it looked more like a typo
than an optimization. It's normally a typo. It's hard to tell the
intent.
I think I'll modify my static checker to ignore these since the typo is
harmless.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: tcrypt - add a missing tab
The "goto out;" line isn't indented far enough. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c index 6ef7815..117f19e 100644 --- a/crypto/tcrypt.c +++ b/crypto/tcrypt.c @@ -629,7 +629,7 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec, printk(KERN_ERR "template (%u) too big for tvmem (%lu)\n", speed[i].blen, TVMEMSIZE * PAGE_SIZE); - goto out; + goto out; } if (speed[i].klen) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: sha1-mb - cleanup a small | vs || typo
|| and | behave basically the same here but || was intended. It causes
a static checker warning when we mix up logical and bitwise operations.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/arch/x86/crypto/sha1-mb/sha1_mb.c
b/arch/x86/crypto/sha1-mb/sha1_mb.c
index 561b286..96d80ad 100644
--- a/arch/x86/crypto/sha1-mb/sha1_mb.c
+++ b/arch/x86/crypto/sha1-mb/sha1_mb.c
@@ -304,7 +304,7 @@ static struct sha1_hash_ctx *sha1_ctx_mgr_submit(struct
sha1_ctx_mgr *mgr,
* Or if the user's buffer contains less than a whole block,
* append as much as possible to the extra block.
*/
- if ((ctx->partial_block_buffer_length) | (len < SHA1_BLOCK_SIZE)) {
+ if ((ctx->partial_block_buffer_length) || (len < SHA1_BLOCK_SIZE)) {
/*
* Compute how many bytes to copy from user buffer into
* extra block
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: sha256-mb - cleanup a || vs | typo
|| and | behave basically the same here but || is intended. It causes a
static checker warning to mix up bitwise and logical operations.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c
b/arch/x86/crypto/sha256-mb/sha256_mb.c
index c9d5dcc..4ec895a 100644
--- a/arch/x86/crypto/sha256-mb/sha256_mb.c
+++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
@@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct
sha256_ctx_mgr *mgr,
* Or if the user's buffer contains less than a whole block,
* append as much as possible to the extra block.
*/
- if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
+ if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) {
/* Compute how many bytes to copy from user buffer into
* extra block
*/
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: drbg - fix an error code in drbg_init_sym_kernel()
We accidentally return PTR_ERR(NULL) which is success but we should
return -ENOMEM.
Fixes: 355912852115 ('crypto: drbg - use CTR AES instead of ECB AES')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/crypto/drbg.c b/crypto/drbg.c
index ded8638..6872d15 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1686,7 +1686,7 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
if (!req) {
pr_info("DRBG: could not allocate request queue\n");
drbg_fini_sym_kernel(drbg);
- return PTR_ERR(req);
+ return -ENOMEM;
}
drbg->ctr_req = req;
skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] crypto: omap-sham - potential Oops on error in probe
On Wed, May 18, 2016 at 01:42:36PM +0300, Peter Ujfalusi wrote: > On 05/18/16 13:39, Dan Carpenter wrote: > > This if statement is reversed so we end up either leaking or Oopsing on > > error. > > Oops, sorry for that. > Probably the other omap-* crypto drivers have the same issue? Can you send a > patch for them or should I do it? I didn't see those static checker warnings so probably it means I can't compile the drivers. Could you do it? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: omap-sham - potential Oops on error in probe
This if statement is reversed so we end up either leaking or Oopsing on
error.
Fixes: dbe246209bc1 ('crypto: omap-sham - Use dma_request_chan() for requesting
DMA channel')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 6eefaa2..63464e8 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -1986,7 +1986,7 @@ err_algs:
>pdata->algs_info[i].algs_list[j]);
err_pm:
pm_runtime_disable(dev);
- if (dd->polling_mode)
+ if (!dd->polling_mode)
dma_release_channel(dd->dma_lch);
data_err:
dev_err(dev, "initialization failed.\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 2/2] crypto: mxc-scc - fix unwinding in mxc_scc_crypto_register()
There are two issues here:
1) We need to decrement "i" otherwise we unregister something that was
not successfully registered.
2) The original code did not unregister the first element in the array
where i is zero.
Fixes: d293b640ebd5 ('crypto: mxc-scc - add basic driver for the MXC SCC')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/mxc-scc.c b/drivers/crypto/mxc-scc.c
index 9b348a7..ff383ef 100644
--- a/drivers/crypto/mxc-scc.c
+++ b/drivers/crypto/mxc-scc.c
@@ -616,7 +616,7 @@ static struct mxc_scc_crypto_tmpl *scc_crypto_algs[] = {
static int mxc_scc_crypto_register(struct mxc_scc *scc)
{
- unsigned int i;
+ int i;
int err = 0;
for (i = 0; i < ARRAY_SIZE(scc_crypto_algs); i++) {
@@ -629,7 +629,7 @@ static int mxc_scc_crypto_register(struct mxc_scc *scc)
return 0;
err_out:
- for (; i > 0; i--)
+ while (--i >= 0)
crypto_unregister_alg(_crypto_algs[i]->alg);
return err;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/2] crypto: mxc-scc - signedness bugs in mxc_scc_ablkcipher_req_init()
->src_nents and ->dst_nents are unsigned so they can't be less than
zero. I fixed this by introducing a temporary "nents" variable.
Fixes: d293b640ebd5 ('crypto: mxc-scc - add basic driver for the MXC SCC')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/crypto/mxc-scc.c b/drivers/crypto/mxc-scc.c
index 38b01bf..9b348a7 100644
--- a/drivers/crypto/mxc-scc.c
+++ b/drivers/crypto/mxc-scc.c
@@ -210,18 +210,21 @@ static int mxc_scc_ablkcipher_req_init(struct
ablkcipher_request *req,
struct mxc_scc_ctx *ctx)
{
struct mxc_scc *scc = ctx->scc;
+ int nents;
- ctx->src_nents = sg_nents_for_len(req->src, req->nbytes);
- if (ctx->src_nents < 0) {
+ nents = sg_nents_for_len(req->src, req->nbytes);
+ if (nents < 0) {
dev_err(scc->dev, "Invalid number of src SC");
- return ctx->src_nents;
+ return nents;
}
+ ctx->src_nents = nents;
- ctx->dst_nents = sg_nents_for_len(req->dst, req->nbytes);
- if (ctx->dst_nents < 0) {
+ nents = sg_nents_for_len(req->dst, req->nbytes);
+ if (nents < 0) {
dev_err(scc->dev, "Invalid number of dst SC");
- return ctx->dst_nents;
+ return nents;
}
+ ctx->dst_nents = nents;
ctx->size = 0;
ctx->offset = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: marvell/cesa - remove unneeded condition
creq->cache[] is an array inside the struct, it's not a pointer and it can't be NULL. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c index 7ca2e0f..7a5058d 100644 --- a/drivers/crypto/marvell/hash.c +++ b/drivers/crypto/marvell/hash.c @@ -768,8 +768,7 @@ static int mv_cesa_ahash_export(struct ahash_request *req, void *hash, *len = creq->len; memcpy(hash, creq->state, digsize); memset(cache, 0, blocksize); - if (creq->cache) - memcpy(cache, creq->cache, creq->cache_ptr); + memcpy(cache, creq->cache, creq->cache_ptr); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
