Re: [RFC v1 PATCH 1/3] drivers: soc: add support for soc_device_match returning -EPROBE_DEFER
On Mon, Apr 19, 2021 at 10:20:13AM +0200, Geert Uytterhoeven wrote:
> Hi Alice,
>
> CC Arnd (soc_device_match() author)
>
> On Mon, Apr 19, 2021 at 6:28 AM Alice Guo (OSS) wrote:
> > From: Alice Guo
> >
> > In i.MX8M boards, the registration of SoC device is later than caam
> > driver which needs it. Caam driver needs soc_device_match to provide
> > -EPROBE_DEFER when no SoC device is registered and no
> > early_soc_dev_attr.
>
> I'm wondering if this is really a good idea: soc_device_match() is a
> last-resort low-level check, and IMHO should be made available early on,
> so there is no need for -EPROBE_DEFER.
>
> >
> > Signed-off-by: Alice Guo
>
> Thanks for your patch!
>
> > --- a/drivers/base/soc.c
> > +++ b/drivers/base/soc.c
> > @@ -110,6 +110,7 @@ static void soc_release(struct device *dev)
> > }
> >
> > static struct soc_device_attribute *early_soc_dev_attr;
> > +static bool soc_dev_attr_init_done = false;
>
> Do you need this variable?
>
> >
> > struct soc_device *soc_device_register(struct soc_device_attribute
> > *soc_dev_attr)
> > {
> > @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct
> > soc_device_attribute *soc_dev_attr
> > return ERR_PTR(ret);
> > }
> >
> > + soc_dev_attr_init_done = true;
> > return soc_dev;
> >
> > out3:
> > @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match(
> > if (!matches)
> > return NULL;
> >
> > + if (!soc_dev_attr_init_done && !early_soc_dev_attr)
>
> if (!soc_bus_type.p && !early_soc_dev_attr)
There is one place checking this already. We could wrap it in a helper
function:
static bool device_init_done(void)
{
return soc_bus_type.p ? true : false;
}
regards,
dan carpenter
Re: [PATCH] crypto: rng - fix crypto_rng_reset() refcounting when !CRYPTO_STATS
On Sun, Mar 21, 2021 at 11:00:09PM -0700, Eric Biggers wrote:
> On Mon, Mar 22, 2021 at 08:45:22AM +0300, Dan Carpenter wrote:
> > On Sun, Mar 21, 2021 at 10:07:48PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers
> > >
> > > crypto_stats_get() is a no-op when the kernel is compiled without
> > > CONFIG_CRYPTO_STATS, so pairing it with crypto_alg_put() unconditionally
> > > (as crypto_rng_reset() does) is wrong.
> > >
> >
> > Presumably the intention was that _get() and _put() should always pair.
> > It's really ugly and horrible that they don't. We could have
> > predicted bug like this would happen and will continue to happen until
> > the crypto_stats_get() is renamed.
> >
>
> Well, the crypto stats stuff has always been pretty broken, so I don't think
> people have looked at it too closely. Currently crypto_stats_get() pairs with
> one of the functions that tallies the statistics, such as
> crypto_stats_rng_seed() or crypto_stats_aead_encrypt(). What change are you
> suggesting, exactly? Maybe moving the conditional crypto_alg_put() into a new
> function crypto_stats_put() and moving it into the callers? Or do you think
> the
> functions should just be renamed to something like crypto_stats_begin() and
> crypto_stats_end_{rng_seed,aead_encrypt}()?
To be honest, I misread the crypto_alg_put() thinking that it was
crypto_*stats*_put(). My favourite fix would be to introduce a
crypto_stats_put() which is a mirror of crypto_stats_get() and ifdeffed
out if we don't have CONFIG_CRYPTO_STATS.
regards,
dan carpenter
Re: [PATCH] crypto: rng - fix crypto_rng_reset() refcounting when !CRYPTO_STATS
On Sun, Mar 21, 2021 at 10:07:48PM -0700, Eric Biggers wrote: > From: Eric Biggers > > crypto_stats_get() is a no-op when the kernel is compiled without > CONFIG_CRYPTO_STATS, so pairing it with crypto_alg_put() unconditionally > (as crypto_rng_reset() does) is wrong. > Presumably the intention was that _get() and _put() should always pair. It's really ugly and horrible that they don't. We could have predicted bug like this would happen and will continue to happen until the crypto_stats_get() is renamed. regards, dan carpenter
Re: [PATCH] crypto: qat - fix use of 'dma_map_single'
Hi Hui,
url:
https://github.com/0day-ci/linux/commits/Hui-Tang/crypto-qat-fix-use-of-dma_map_single/20210301-114717
base:
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-randconfig-m021-20210304 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/crypto/qat/qat_common/qat_algs.c:809 qat_alg_sgl_to_bufl() error:
uninitialized symbol 'blp'.
vim +/blp +809 drivers/crypto/qat/qat_common/qat_algs.c
d370cec3219490 Tadeusz Struk 2014-06-05 711 static int
qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst,
d370cec3219490 Tadeusz Struk 2014-06-05 712
struct scatterlist *sgl,
e19ab1211d2848 Herbert Xu 2015-07-30 713
struct scatterlist *sglout,
d370cec3219490 Tadeusz Struk 2014-06-05 714
struct qat_crypto_request *qat_req)
d370cec3219490 Tadeusz Struk 2014-06-05 715 {
d370cec3219490 Tadeusz Struk 2014-06-05 716 struct device *dev =
&GET_DEV(inst->accel_dev);
e19ab1211d2848 Herbert Xu 2015-07-30 717 int i, sg_nctr = 0;
e19ab1211d2848 Herbert Xu 2015-07-30 718 int n = sg_nents(sgl);
d370cec3219490 Tadeusz Struk 2014-06-05 719 struct qat_alg_buf_list
*bufl;
d370cec3219490 Tadeusz Struk 2014-06-05 720 struct qat_alg_buf_list
*buflout = NULL;
d370cec3219490 Tadeusz Struk 2014-06-05 721 dma_addr_t blp;
^^
d370cec3219490 Tadeusz Struk 2014-06-05 722 dma_addr_t bloutp = 0;
d370cec3219490 Tadeusz Struk 2014-06-05 723 struct scatterlist *sg;
1793d1aba19415 Gustavo A. R. Silva 2019-06-06 724 size_t sz_out, sz =
struct_size(bufl, bufers, n + 1);
d370cec3219490 Tadeusz Struk 2014-06-05 725
d370cec3219490 Tadeusz Struk 2014-06-05 726 if (unlikely(!n))
d370cec3219490 Tadeusz Struk 2014-06-05 727 return -EINVAL;
d370cec3219490 Tadeusz Struk 2014-06-05 728
82f82504b8f5f1 Tadeusz Struk 2014-12-08 729 bufl = kzalloc_node(sz,
GFP_ATOMIC,
09adc8789c4e89 Tadeusz Struk 2014-10-13 730
dev_to_node(&GET_DEV(inst->accel_dev)));
d370cec3219490 Tadeusz Struk 2014-06-05 731 if (unlikely(!bufl))
d370cec3219490 Tadeusz Struk 2014-06-05 732 return -ENOMEM;
d370cec3219490 Tadeusz Struk 2014-06-05 733
d370cec3219490 Tadeusz Struk 2014-06-05 734 for_each_sg(sgl, sg, n,
i) {
e19ab1211d2848 Herbert Xu 2015-07-30 735 int y = sg_nctr;
82f82504b8f5f1 Tadeusz Struk 2014-12-08 736
82f82504b8f5f1 Tadeusz Struk 2014-12-08 737 if (!sg->length)
82f82504b8f5f1 Tadeusz Struk 2014-12-08 738
continue;
d65071ecde1ed1 Tadeusz Struk 2014-06-24 739
d370cec3219490 Tadeusz Struk 2014-06-05 740
bufl->bufers[y].addr = dma_map_single(dev, sg_virt(sg),
d370cec3219490 Tadeusz Struk 2014-06-05 741
sg->length,
d370cec3219490 Tadeusz Struk 2014-06-05 742
DMA_BIDIRECTIONAL);
d370cec3219490 Tadeusz Struk 2014-06-05 743
bufl->bufers[y].len = sg->length;
d370cec3219490 Tadeusz Struk 2014-06-05 744 if
(unlikely(dma_mapping_error(dev, bufl->bufers[y].addr)))
72eed063767e13 Arnd Bergmann 2017-06-22 745 goto
err_in;
"blp" uninitialized at this goto.
82f82504b8f5f1 Tadeusz Struk 2014-12-08 746 sg_nctr++;
d370cec3219490 Tadeusz Struk 2014-06-05 747 }
e19ab1211d2848 Herbert Xu 2015-07-30 748 bufl->num_bufs =
sg_nctr;
54bc41cf4d0517 Hui Tang2021-03-01 749 blp =
dma_map_single(dev, bufl, sz, DMA_TO_DEVICE);
^^
Initialized here.
54bc41cf4d0517 Hui Tang2021-03-01 750 if
(unlikely(dma_mapping_error(dev, blp)))
54bc41cf4d0517 Hui Tang2021-03-01 751 goto err_in;
d370cec3219490 Tadeusz Struk 2014-06-05 752 qat_req->buf.bl = bufl;
d370cec3219490 Tadeusz Struk 2014-06-05 753 qat_req->buf.blp = blp;
d370cec3219490 Tadeusz Struk 2014-06-05 754 qat_req->buf.sz = sz;
d370cec3219490 Tadeusz Struk 2014-06-05 755 /* Handle out of place
operation */
d370cec3219490 Tadeusz Struk 2014-06-05 756 if (sgl != sglout) {
d370cec3219490 Tadeusz Struk 2014-06-05 757 struct
qat_alg_buf *bufers;
d370cec3219490 Tadeusz Struk 2014
[PATCH] crypto: octeontx2 - fix signedness bug in cptvf_register_interrupts()
The "num_vec" has to be signed for the error handling to work.
Fixes: 19d8e8c7be15 ("crypto: octeontx2 - add virtual function driver support")
Signed-off-by: Dan Carpenter
---
drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
index 9663be38ee40..47f378731024 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
@@ -34,7 +34,7 @@ static void cptvf_disable_pfvf_mbox_intrs(struct
otx2_cptvf_dev *cptvf)
static int cptvf_register_interrupts(struct otx2_cptvf_dev *cptvf)
{
int ret, irq;
- u32 num_vec;
+ int num_vec;
num_vec = pci_msix_vec_count(cptvf->pdev);
if (num_vec <= 0)
--
2.29.2
[PATCH] crypto: keembay-ocs-hcu - Fix a WARN() message
The first argument to WARN() is a condition and the messages is the
second argument is the string, so this WARN() will only display the
__func__ part of the message.
Fixes: ae832e329a8d ("crypto: keembay-ocs-hcu - Add HMAC support")
Signed-off-by: Dan Carpenter
---
drivers/crypto/keembay/keembay-ocs-hcu-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/keembay/keembay-ocs-hcu-core.c
b/drivers/crypto/keembay/keembay-ocs-hcu-core.c
index d547af047131..c4b97b4160e9 100644
--- a/drivers/crypto/keembay/keembay-ocs-hcu-core.c
+++ b/drivers/crypto/keembay/keembay-ocs-hcu-core.c
@@ -388,7 +388,7 @@ static int prepare_ipad(struct ahash_request *req)
* longer keys are hashed by kmb_ocs_hcu_setkey()).
*/
if (ctx->key_len > rctx->blk_sz) {
- WARN("%s: Invalid key length in tfm context\n", __func__);
+ WARN(1, "%s: Invalid key length in tfm context\n", __func__);
return -EINVAL;
}
memzero_explicit(&ctx->key[ctx->key_len],
--
2.29.2
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
This should not generate a warning.
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
>
> There's 3 other cases that don't quite match between GCC and Clang I
> observe in the kernel:
> switch (x) {
> case 0:
> ++x;
> default:
> goto y;
> }
> y:;
This should generate a warning.
>
> switch (x) {
> case 0:
> ++x;
> default:
> return;
> }
Warn for this.
>
> switch (x) {
> case 0:
> ++x;
> default:
> ;
> }
Don't warn for this.
If adding a break statement changes the flow of the code then warn about
potentially missing break statements, but if it doesn't change anything
then don't warn about it.
regards,
dan carpenter
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook wrote: > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > > This series aims to fix almost all remaining fall-through warnings in > > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > > letting the code fall through to the next case. > > > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > > to move in that direction. > > > > > > > > > > Something important to mention is that there is currently a > > > > > discrepancy > > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > > case > > > > > statements or to cases that only contain a break/continue/return > > > > > statement[2][3][4]. > > > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > > find? > > > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > > > It's certainly a place where the intent is not always clear. I think > > > this makes all the cases unambiguous, and doesn't impact the machine > > > code, since the compiler will happily optimize away any behavioral > > > redundancy. > > > > If none of the 140 patches here fix a real bug, and there is no change > > to machine code then it sounds to me like a W=2 kind of a warning. > > FWIW, this series has found at least one bug so far: > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ This is a fallthrough to a return and not to a break. That should trigger a warning. The fallthrough to a break should not generate a warning. The bug we're trying to fix is "missing break statement" but if the result of the bug is "we hit a break statement" then now we're just talking about style. GCC should limit itself to warning about potentially buggy code. regards, dan carpenter
Re: [PATCH 00/18] use semicolons rather than commas to separate statements
On Tue, Sep 29, 2020 at 02:20:00PM +0200, Ard Biesheuvel wrote:
> On Sun, 27 Sep 2020 at 21:56, Julia Lawall wrote:
> >
> > These patches replace commas by semicolons.
>
>
> Why?
>
In the best case, these commas are just uninitentional mess, like typing
an extra space character or something. I've looked at them before and
one case I see where they are introduced is when people convert a
struct initializer to code.
- struct foo {
- .a = 1,
- .b = 2,
...
+ foo.a = 1,
+ foo.b = 2,
The times where commas are used deliberately to replace curly braces are
just evil. Either way the code is cleaner with semi-colons.
regards,
dan carpenter
[PATCH] crypto: sa2ul - Fix pm_runtime_get_sync() error checking
The pm_runtime_get_sync() function returns either 0 or 1 on success but
this code treats a return of 1 as a failure.
Fixes: 7694b6ca649f ("crypto: sa2ul - Add crypto driver")
Signed-off-by: Dan Carpenter
---
drivers/crypto/sa2ul.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/sa2ul.c b/drivers/crypto/sa2ul.c
index acabb8ddacb6..604798c65e85 100644
--- a/drivers/crypto/sa2ul.c
+++ b/drivers/crypto/sa2ul.c
@@ -2333,7 +2333,7 @@ static int sa_ul_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
- if (ret) {
+ if (ret < 0) {
dev_err(&pdev->dev, "%s: failed to get sync: %d\n", __func__,
ret);
return ret;
--
2.28.0
[PATCH] crypto/chtls: Fix double free in chtls_pass_accept_request()
The chtls_recv_sock() function frees "oreq" so the free here is a double
free.
Fixes: 6abde0b24122 ("crypto/chtls: IPv6 support for inline TLS")
Signed-off-by: Dan Carpenter
---
drivers/crypto/chelsio/chtls/chtls_cm.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c
b/drivers/crypto/chelsio/chtls/chtls_cm.c
index 05520dccd906..140342024bd1 100644
--- a/drivers/crypto/chelsio/chtls/chtls_cm.c
+++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
@@ -1381,7 +1381,7 @@ static void chtls_pass_accept_request(struct sock *sk,
newsk = chtls_recv_sock(sk, oreq, network_hdr, req, cdev);
if (!newsk)
- goto free_oreq;
+ goto reject;
if (chtls_get_module(newsk))
goto reject;
@@ -1397,8 +1397,6 @@ static void chtls_pass_accept_request(struct sock *sk,
kfree_skb(skb);
return;
-free_oreq:
- chtls_reqsk_free(oreq);
reject:
mk_tid_release(reply_skb, 0, tid);
cxgb4_ofld_send(cdev->lldi->ports[0], reply_skb);
--
2.28.0
Re: [PATCH v4 2/3] mm, treewide: Rename kzfree() to kfree_sensitive()
Last time you sent this we couldn't decide which tree it should go through. Either the crypto tree or through Andrew seems like the right thing to me. Also the other issue is that it risks breaking things if people add new kzfree() instances while we are doing the transition. Could you just add a "#define kzfree kfree_sensitive" so that things continue to compile and we can remove it in the next kernel release? regards, dan carpenter
Re: [PATCH v4 1/3] mm/slab: Use memzero_explicit() in kzfree()
On Tue, Jun 16, 2020 at 08:42:08AM +0200, Michal Hocko wrote:
> On Mon 15-06-20 21:57:16, Waiman Long wrote:
> > The kzfree() function is normally used to clear some sensitive
> > information, like encryption keys, in the buffer before freeing it back
> > to the pool. Memset() is currently used for the buffer clearing. However,
> > it is entirely possible that the compiler may choose to optimize away the
> > memory clearing especially if LTO is being used. To make sure that this
> > optimization will not happen, memzero_explicit(), which is introduced
> > in v3.18, is now used in kzfree() to do the clearing.
> >
> > Fixes: 3ef0e5ba4673 ("slab: introduce kzfree()")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Waiman Long
>
> Acked-by: Michal Hocko
>
> Although I am not really sure this is a stable material. Is there any
> known instance where the memset was optimized out from kzfree?
I told him to add the stable. Otherwise it will just get reported to
me again. It's a just safer to backport it before we forget.
regards,
dan carpenter
Re: [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon, Apr 13, 2020 at 05:15:49PM -0400, Waiman Long wrote:
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 23c7500eea7d..c08bc7eb20bd 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1707,17 +1707,17 @@ void *krealloc(const void *p, size_t new_size, gfp_t
> flags)
> EXPORT_SYMBOL(krealloc);
>
> /**
> - * kzfree - like kfree but zero memory
> + * kfree_sensitive - Clear sensitive information in memory before freeing
> * @p: object to free memory of
> *
> * The memory of the object @p points to is zeroed before freed.
> - * If @p is %NULL, kzfree() does nothing.
> + * If @p is %NULL, kfree_sensitive() does nothing.
> *
> * Note: this function zeroes the whole allocated buffer which can be a good
> * deal bigger than the requested buffer size passed to kmalloc(). So be
> * careful when using this function in performance sensitive code.
> */
> -void kzfree(const void *p)
> +void kfree_sensitive(const void *p)
> {
> size_t ks;
> void *mem = (void *)p;
> @@ -1725,10 +1725,10 @@ void kzfree(const void *p)
> if (unlikely(ZERO_OR_NULL_PTR(mem)))
> return;
> ks = ksize(mem);
> - memset(mem, 0, ks);
> + memzero_explicit(mem, ks);
^
This is an unrelated bug fix. It really needs to be pulled into a
separate patch by itself and back ported to stable kernels.
> kfree(mem);
> }
> -EXPORT_SYMBOL(kzfree);
> +EXPORT_SYMBOL(kfree_sensitive);
>
> /**
> * ksize - get the actual amount of memory allocated for a given object
regards,
dan carpenter
[PATCH] crypto: marvell/octeontx - Fix a potential NULL dereference
Smatch reports that:
drivers/crypto/marvell/octeontx/otx_cptvf_algs.c:132 otx_cpt_aead_callback()
warn: variable dereferenced before check 'cpt_info' (see line 121)
This function is called from process_pending_queue() as:
drivers/crypto/marvell/octeontx/otx_cptvf_reqmgr.c
599 /*
600 * Call callback after current pending entry has been
601 * processed, we don't do it if the callback pointer is
602 * invalid.
603 */
604 if (callback)
605 callback(res_code, areq, cpt_info);
It does appear to me that "cpt_info" can be NULL so this could lead to
a NULL dereference.
Fixes: 10b4f09491bf ("crypto: marvell - add the Virtual Function driver for
CPT")
Signed-off-by: Dan Carpenter
---
drivers/crypto/marvell/octeontx/otx_cptvf_algs.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
b/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
index 60e744f680d34..1e0a1d70ebd39 100644
--- a/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
+++ b/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
@@ -118,6 +118,9 @@ static void otx_cpt_aead_callback(int status, void *arg1,
void *arg2)
struct otx_cpt_req_info *cpt_req;
struct pci_dev *pdev;
+ if (!cpt_info)
+ goto complete;
+
cpt_req = cpt_info->req;
if (!status) {
/*
@@ -129,10 +132,10 @@ static void otx_cpt_aead_callback(int status, void *arg1,
void *arg2)
!cpt_req->is_enc)
status = validate_hmac_cipher_null(cpt_req);
}
- if (cpt_info) {
- pdev = cpt_info->pdev;
- do_request_cleanup(pdev, cpt_info);
- }
+ pdev = cpt_info->pdev;
+ do_request_cleanup(pdev, cpt_info);
+
+complete:
if (areq)
areq->complete(areq, status);
}
--
2.26.2
Re: [PATCH v2] crypto: hisilicon - allow smaller reads in debugfs
On Fri, Jun 05, 2020 at 09:19:53AM +0800, Shukun Tan wrote: > Hi Dan, > > On 2020/6/2 21:54, Dan Carpenter wrote: > > Originally this code rejected any read less than 256 bytes. There > > is no need for this artificial limit. We should just use the normal > > helper functions to read a string from the kernel. > > > > Signed-off-by: Dan Carpenter > > --- > > v2: Use simple_read_from_buffer(). The v1 was slightly half arsed > > because I left the original check for: > > > > if (*pos) > > return 0; > > > > So it could result in partial reads. The new code means that if you > > want to read the buffer one byte at a time, that's fine or if you want > > to read it in one 256 byte chunk that's also fine. Plus it deletes 21 > > lines of code and is a lot cleaner. > > > > In fact, In our original design, we do not hope the user do the partial reads. > Thank you for your work, but I still insist on adding this limit. This not how POSIX filesystems work... :( Last time you said that this literally breaks cat. This doesn't break anything if the user chooses not to read a single byte at a time. That's obviously a crazy way to read a file. It just allows them to if they want. Or if they want to read 256 bytes at a time then that also works. My patch makes *everything* work. regards, dan carpenter
Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance
On Wed, Jun 03, 2020 at 10:08:56AM +0200, Stephan Müller wrote:
> The Jitter RNG is unconditionally allocated as a seed source follwoing
> the patch 97f2650e5040. Thus, the instance must always be deallocated.
>
> Reported-by: syzbot+2e635807decef724a...@syzkaller.appspotmail.com
> Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> Signed-off-by: Stephan Mueller
> ---
> crypto/drbg.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 37526eb8c5d5..33d28016da2d 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state *drbg)
> if (drbg->random_ready.func) {
> del_random_ready_callback(&drbg->random_ready);
> cancel_work_sync(&drbg->seed_work);
> + }
> +
> + if (drbg->jent) {
> crypto_free_rng(drbg->jent);
> drbg->jent = NULL;
> }
free_everything functions never work. For example, "drbg->jent" can be
an error pointer at this point.
crypto/drbg.c
1577 if (!drbg->core) {
1578 drbg->core = &drbg_cores[coreref];
1579 drbg->pr = pr;
1580 drbg->seeded = false;
1581 drbg->reseed_threshold = drbg_max_requests(drbg);
1582
1583 ret = drbg_alloc_state(drbg);
1584 if (ret)
1585 goto unlock;
1586
1587 ret = drbg_prepare_hrng(drbg);
1588 if (ret)
1589 goto free_everything;
If we hit two failures inside drbg_prepare_hrng() then "drbg->jent" can
be an error pointer.
1590
1591 if (IS_ERR(drbg->jent)) {
1592 ret = PTR_ERR(drbg->jent);
1593 drbg->jent = NULL;
1594 if (fips_enabled || ret != -ENOENT)
1595 goto free_everything;
1596 pr_info("DRBG: Continuing without Jitter
RNG\n");
1597 }
1598
1599 reseed = false;
1600 }
1601
1602 ret = drbg_seed(drbg, pers, reseed);
1603
1604 if (ret && !reseed)
1605 goto free_everything;
1606
1607 mutex_unlock(&drbg->drbg_mutex);
1608 return ret;
1609
1610 unlock:
1611 mutex_unlock(&drbg->drbg_mutex);
1612 return ret;
1613
1614 free_everything:
1615 mutex_unlock(&drbg->drbg_mutex);
1616 drbg_uninstantiate(drbg);
Leading to an Oops.
1617 return ret;
1618 }
regards,
dan carpenter
[PATCH v2] crypto: hisilicon - allow smaller reads in debugfs
Originally this code rejected any read less than 256 bytes. There
is no need for this artificial limit. We should just use the normal
helper functions to read a string from the kernel.
Signed-off-by: Dan Carpenter
---
v2: Use simple_read_from_buffer(). The v1 was slightly half arsed
because I left the original check for:
if (*pos)
return 0;
So it could result in partial reads. The new code means that if you
want to read the buffer one byte at a time, that's fine or if you want
to read it in one 256 byte chunk that's also fine. Plus it deletes 21
lines of code and is a lot cleaner.
drivers/crypto/hisilicon/qm.c | 33 ++---
1 file changed, 6 insertions(+), 27 deletions(-)
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index 9bb263cec6c30..13ccb9e29a2e1 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -1064,19 +1064,10 @@ static ssize_t qm_cmd_read(struct file *filp, char
__user *buffer,
char buf[QM_DBG_READ_LEN];
int len;
- if (*pos)
- return 0;
-
- if (count < QM_DBG_READ_LEN)
- return -ENOSPC;
+ len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
+ "Please echo help to cmd to get help information");
- len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
- "Please echo help to cmd to get help information");
-
- if (copy_to_user(buffer, buf, len))
- return -EFAULT;
-
- return (*pos = len);
+ return simple_read_from_buffer(buffer, count, pos, buf, len);
}
static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2691,24 +2682,12 @@ static ssize_t qm_status_read(struct file *filp, char
__user *buffer,
{
struct hisi_qm *qm = filp->private_data;
char buf[QM_DBG_READ_LEN];
- int val, cp_len, len;
-
- if (*pos)
- return 0;
-
- if (count < QM_DBG_READ_LEN)
- return -ENOSPC;
+ int val, len;
val = atomic_read(&qm->status.flags);
- len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
- if (!len)
- return -EFAULT;
-
- cp_len = copy_to_user(buffer, buf, len);
- if (cp_len)
- return -EFAULT;
+ len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
- return (*pos = len);
+ return simple_read_from_buffer(buffer, count, pos, buf, len);
}
static const struct file_operations qm_status_fops = {
--
2.26.2
[PATCH] crypto: hisilicon/qm - allow smaller reads in debugfs
Originally this code rejected any read less than 256 bytes. There
is no need for this artificial limit.
Also I have changed the snprintf() functions to scnprintf(). The
difference is that snprintf() returns the number of bytes which would
have been copied if there were enough space and scnprintf() returns the
number of bytes which were actually copied. It doesn't matter here
because the strings are very short so they can't go over 256 bytes.
Signed-off-by: Dan Carpenter
---
drivers/crypto/hisilicon/qm.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index a781c02251980..9c0c9f500d91d 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -1076,16 +1076,15 @@ static ssize_t qm_cmd_read(struct file *filp, char
__user *buffer,
if (*pos)
return 0;
- if (count < QM_DBG_READ_LEN)
- return -ENOSPC;
-
- len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
+ len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
"Please echo help to cmd to get help information");
+ len = min_t(size_t, len, count);
if (copy_to_user(buffer, buf, len))
return -EFAULT;
- return (*pos = len);
+ *pos = len;
+ return len;
}
static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2710,19 +2709,18 @@ static ssize_t qm_status_read(struct file *filp, char
__user *buffer,
if (*pos)
return 0;
- if (count < QM_DBG_READ_LEN)
- return -ENOSPC;
-
val = atomic_read(&qm->status.flags);
- len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
+ len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
if (!len)
return -EFAULT;
+ len = min_t(size_t, len, count);
cp_len = copy_to_user(buffer, buf, len);
if (cp_len)
return -EFAULT;
- return (*pos = len);
+ *pos = len;
+ return len;
}
static const struct file_operations qm_status_fops = {
--
2.26.2
[PATCH] Crypto/chcr: drop refcount on error path in chcr_aead_op()
We need to drop inflight counter before returning on this error path.
Fixes: d91a3159e8d9 ("Crypto/chcr: fix gcm-aes and rfc4106-gcm failed tests")
Signed-off-by: Dan Carpenter
---
drivers/crypto/chelsio/chcr_algo.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/crypto/chelsio/chcr_algo.c
b/drivers/crypto/chelsio/chcr_algo.c
index 83ddc2b39490e..e05998a1c0148 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -3744,6 +3744,7 @@ static int chcr_aead_op(struct aead_request *req,
crypto_ipsec_check_assoclen(req->assoclen) != 0) {
pr_err("RFC4106: Invalid value of assoclen %d\n",
req->assoclen);
+ chcr_dec_wrcount(cdev);
return -EINVAL;
}
--
2.26.2
[PATCH] cxgb4/chcr: Fix a leak in chcr_ktls_dev_add()
We need to free "tx_info->l2te" if chcr_setup_connection() fails. My
other concern was that if we free "tx_info" then "tx_ctx->chcr_info"
points to a freed variable. I don't think this causes a problem but
it's cleaner to reset it back to NULL. Also I renamed the labels to
say what the gotos do instead of using numbered labels.
Fixes: 34aba2c45024 ("cxgb4/chcr : Register to tls add and del callback")
Signed-off-by: Dan Carpenter
---
Applies on top of Wei Yongjun's patch.
drivers/crypto/chelsio/chcr_ktls.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/chelsio/chcr_ktls.c
b/drivers/crypto/chelsio/chcr_ktls.c
index baaea8ce40806..3173ac3099bc6 100644
--- a/drivers/crypto/chelsio/chcr_ktls.c
+++ b/drivers/crypto/chelsio/chcr_ktls.c
@@ -478,7 +478,7 @@ static int chcr_ktls_dev_add(struct net_device *netdev,
struct sock *sk,
tx_info->rx_qid = chcr_get_first_rx_qid(adap);
if (unlikely(tx_info->rx_qid < 0))
- goto out2;
+ goto free_tx_info;
tx_info->prev_seq = start_offload_tcp_sn;
tx_info->tcp_start_seq_number = start_offload_tcp_sn;
@@ -486,7 +486,7 @@ static int chcr_ktls_dev_add(struct net_device *netdev,
struct sock *sk,
/* save crypto keys */
ret = chcr_ktls_save_keys(tx_info, crypto_info, direction);
if (ret < 0)
- goto out2;
+ goto free_tx_info;
/* get peer ip */
if (sk->sk_family == AF_INET ||
@@ -502,14 +502,14 @@ static int chcr_ktls_dev_add(struct net_device *netdev,
struct sock *sk,
if (!dst) {
pr_err("DST entry not found\n");
ret = -ENOENT;
- goto out2;
+ goto free_tx_info;
}
n = dst_neigh_lookup(dst, daaddr);
if (!n || !n->dev) {
pr_err("neighbour not found\n");
dst_release(dst);
ret = -ENOENT;
- goto out2;
+ goto free_tx_info;
}
tx_info->l2te = cxgb4_l2t_get(adap->l2t, n, n->dev, 0);
@@ -519,7 +519,7 @@ static int chcr_ktls_dev_add(struct net_device *netdev,
struct sock *sk,
if (!tx_info->l2te) {
pr_err("l2t entry not found\n");
ret = -ENOENT;
- goto out2;
+ goto free_tx_info;
}
tx_ctx->chcr_info = tx_info;
@@ -529,12 +529,16 @@ static int chcr_ktls_dev_add(struct net_device *netdev,
struct sock *sk,
*/
ret = chcr_setup_connection(sk, tx_info);
if (ret)
- goto out2;
+ goto free_l2te;
atomic64_inc(&adap->chcr_stats.ktls_tx_connection_open);
return 0;
-out2:
+
+free_l2te:
+ cxgb4_l2t_release(tx_info->l2te);
+free_tx_info:
kvfree(tx_info);
+ tx_ctx->chcr_info = NULL;
out:
atomic64_inc(&adap->chcr_stats.ktls_tx_connection_fail);
return ret;
--
2.26.2
[bug report] crypto: dh - Add DH software implementation
Hello Salvatore Benedetto,
The patch 802c7f1c84e4: "crypto: dh - Add DH software implementation"
from Jun 22, 2016, leads to the following static checker warning:
crypto/dh_helper.c:99 crypto_dh_decode_key()
warn: potential overflow
crypto/dh_helper.c
68 int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh
*params)
69 {
70 const u8 *ptr = buf;
71 struct kpp_secret secret;
72
73 if (unlikely(!buf || len < DH_KPP_SECRET_MIN_SIZE))
74 return -EINVAL;
75
76 ptr = dh_unpack_data(&secret, ptr, sizeof(secret));
77 if (secret.type != CRYPTO_KPP_SECRET_TYPE_DH)
78 return -EINVAL;
79
80 ptr = dh_unpack_data(¶ms->key_size, ptr,
sizeof(params->key_size));
81 ptr = dh_unpack_data(¶ms->p_size, ptr,
sizeof(params->p_size));
82 ptr = dh_unpack_data(¶ms->q_size, ptr,
sizeof(params->q_size));
83 ptr = dh_unpack_data(¶ms->g_size, ptr,
sizeof(params->g_size));
84 if (secret.len != crypto_dh_key_len(params))
The largest parameter has to be "params->p_size" but it's a u32 from the
user. So crypto_dh_key_len() can have an integer overflow and wrap back
to "secret.len".
85 return -EINVAL;
86
87 /*
88 * Don't permit the buffer for 'key' or 'g' to be larger than
'p', since
89 * some drivers assume otherwise.
90 */
91 if (params->key_size > params->p_size ||
92 params->g_size > params->p_size || params->q_size >
params->p_size)
This ensures that "params->p_size" is the largest.
93 return -EINVAL;
94
95 /* Don't allocate memory. Set pointers to data within
96 * the given buffer
97 */
98 params->key = (void *)ptr;
99 params->p = (void *)(ptr + params->key_size);
100 params->q = (void *)(ptr + params->key_size + params->p_size);
This could wrap.
101 params->g = (void *)(ptr + params->key_size + params->p_size +
102 params->q_size);
103
104 /*
105 * Don't permit 'p' to be 0. It's not a prime number, and it's
subject
106 * to corner cases such as 'mod 0' being undefined or
107 * crypto_kpp_maxsize() returning 0.
108 */
109 if (memchr_inv(params->p, 0, params->p_size) == NULL)
It would probably/hopefully lead to an Oops in memchr_inv().
110 return -EINVAL;
111
112 /* It is permissible to not provide Q. */
113 if (params->q_size == 0)
114 params->q = NULL;
115
116 return 0;
117 }
regards,
dan carpenter
Re: [PATCH] crypto: qat - Endian bug in interrupt handler
Never mind. Please ignore this patch. This is Intel hardware so it's little endian. There are a bunch of other test_bit() casts which would be problematic so this wouldn't really fix anything anyway. regards, dan carpenter
[PATCH] crypto: qat - Endian bug in interrupt handler
The "vf_mask" as a u32 but we were casting it to unsigned long when we
do the for_each_set_bit() loop. The problem is that is an out of bounds
read on big endian 64 bit systems.
Fixes: ed8ccaef52fa ("crypto: qat - Add support for SRIOV")
Signed-off-by: Dan Carpenter
---
Not tested. I don't know if 64bit big endian support matters...
drivers/crypto/qat/qat_common/adf_isr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/qat/qat_common/adf_isr.c
b/drivers/crypto/qat/qat_common/adf_isr.c
index cd1cdf5305bc..50661f0a6ed4 100644
--- a/drivers/crypto/qat/qat_common/adf_isr.c
+++ b/drivers/crypto/qat/qat_common/adf_isr.c
@@ -111,7 +111,7 @@ static irqreturn_t adf_msix_isr_ae(int irq, void *dev_ptr)
struct adf_bar *pmisc =
&GET_BARS(accel_dev)[hw_data->get_misc_bar_id(hw_data)];
void __iomem *pmisc_bar_addr = pmisc->virt_addr;
- u32 vf_mask;
+ unsigned long vf_mask;
/* Get the interrupt sources triggered by VFs */
vf_mask = ((ADF_CSR_RD(pmisc_bar_addr, ADF_ERRSOU5) &
@@ -132,8 +132,7 @@ static irqreturn_t adf_msix_isr_ae(int irq, void *dev_ptr)
* unless the VF is malicious and is attempting to
* flood the host OS with VF2PF interrupts.
*/
- for_each_set_bit(i, (const unsigned long *)&vf_mask,
-(sizeof(vf_mask) * BITS_PER_BYTE)) {
+ for_each_set_bit(i, &vf_mask, 32) {
vf_info = accel_dev->pf.vf_info + i;
if (!__ratelimit(&vf_info->vf2pf_ratelimit)) {
--
2.20.1
[bug report] crypto: qat - Intel(R) QAT driver framework
Hello Tadeusz Struk,
The patch d8cba25d2c68: "crypto: qat - Intel(R) QAT driver framework"
from Jun 5, 2014, leads to the following static checker warning:
drivers/crypto/qat/qat_common/adf_ctl_drv.c:159 adf_add_key_value_data()
warn: 'adf_cfg_add_key_value_param' unterminated user string
'key_val->key'
drivers/crypto/qat/qat_common/adf_ctl_drv.c
151 static int adf_add_key_value_data(struct adf_accel_dev *accel_dev,
152const char *section,
153const struct adf_user_cfg_key_val
*key_val)
154 {
155 if (key_val->type == ADF_HEX) {
156 long *ptr = (long *)key_val->val;
157 long val = *ptr;
158
159 if (adf_cfg_add_key_value_param(accel_dev, section,
160 key_val->key, (void
*)val,
Not terminated. We end up adding the named item into a list. Then we
look it up but when we're looking it up, we don't ensure that those
strings are NUL terminated either so there is a potential that it
overflows beyond the end of the array.
161 key_val->type)) {
162 dev_err(&GET_DEV(accel_dev),
163 "failed to add hex keyvalue.\n");
164 return -EFAULT;
165 }
166 } else {
167 if (adf_cfg_add_key_value_param(accel_dev, section,
168 key_val->key,
key_val->val,
169 key_val->type)) {
170 dev_err(&GET_DEV(accel_dev),
171 "failed to add keyvalue.\n");
172 return -EFAULT;
173 }
174 }
175 return 0;
176 }
[ snip ]
203 while (params_head) {
204 if (copy_from_user(&key_val, (void __user
*)params_head,
Gets set here.
205 sizeof(key_val))) {
206 dev_err(&GET_DEV(accel_dev),
207 "Failed to copy keyvalue.\n");
208 goto out_err;
209 }
210 if (adf_add_key_value_data(accel_dev,
section.name,
211 &key_val)) {
212 goto out_err;
213 }
214 params_head = key_val.next;
215 }
See also:
drivers/crypto/qat/qat_common/adf_ctl_drv.c:159 adf_add_key_value_data() warn:
'adf_cfg_add_key_value_param' unterminated user string 'key_val->key'
drivers/crypto/qat/qat_common/adf_ctl_drv.c:167 adf_add_key_value_data() warn:
'adf_cfg_add_key_value_param' unterminated user string 'key_val->key'
drivers/crypto/qat/qat_common/adf_ctl_drv.c:167 adf_add_key_value_data() warn:
'adf_cfg_add_key_value_param' unterminated user string 'key_val->val'
drivers/crypto/qat/qat_common/adf_ctl_drv.c:195 adf_copy_key_value_data() warn:
'adf_cfg_section_add' unterminated user string 'section.name'
regards,
dan carpenter
potential underfow in crypto/lrw.c setkey() setkey
crypto/lrw.c
72 static int setkey(struct crypto_skcipher *parent, const u8 *key,
73unsigned int keylen)
74 {
75 struct priv *ctx = crypto_skcipher_ctx(parent);
76 struct crypto_skcipher *child = ctx->child;
77 int err, bsize = LRW_BLOCK_SIZE;
78 const u8 *tweak = key + keylen - bsize;
Smatch thinks that keylen is user controlled from zero to some upper
bound. How do we know it's >= LRW_BLOCK_SIZE (16)?
I find the crypto code sort of hard to follow... There are a bunch of
setkey pointers and they're sometimes called recursively. Is there
some trick or hints?
79 be128 tmp = { 0 };
80 int i;
81
82 crypto_skcipher_clear_flags(child, CRYPTO_TFM_REQ_MASK);
83 crypto_skcipher_set_flags(child,
crypto_skcipher_get_flags(parent) &
84 CRYPTO_TFM_REQ_MASK);
85 err = crypto_skcipher_setkey(child, key, keylen - bsize);
86 crypto_skcipher_set_flags(parent,
crypto_skcipher_get_flags(child) &
87CRYPTO_TFM_RES_MASK);
88 if (err)
89 return err;
90
91 if (ctx->table)
92 gf128mul_free_64k(ctx->table);
93
94 /* initialize multiplication table for Key2 */
95 ctx->table = gf128mul_init_64k_bbe((be128 *)tweak);
96 if (!ctx->table)
97 return -ENOMEM;
98
99 /* initialize optimization table */
100 for (i = 0; i < 128; i++) {
101 setbit128_bbe(&tmp, i);
102 ctx->mulinc[i] = tmp;
103 gf128mul_64k_bbe(&ctx->mulinc[i], ctx->table);
104 }
105
106 return 0;
107 }
regards,
dan carpenter
[PATCH] crypto: caam/qi - Change a couple IS_ERR_OR_NULL() checks to IS_ERR()
create_caam_req_fq() doesn't return NULL pointers so there is no need to
check. The NULL checks are problematic because it's hard to say how a
NULL return should be handled, so removing the checks is a nice cleanup.
Signed-off-by: Dan Carpenter
---
drivers/crypto/caam/qi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c
index 7cb8b1755e57..9f08f84cca59 100644
--- a/drivers/crypto/caam/qi.c
+++ b/drivers/crypto/caam/qi.c
@@ -318,7 +318,7 @@ int caam_drv_ctx_update(struct caam_drv_ctx *drv_ctx, u32
*sh_desc)
/* Create a new req FQ in parked state */
new_fq = create_caam_req_fq(drv_ctx->qidev, drv_ctx->rsp_fq,
drv_ctx->context_a, 0);
- if (IS_ERR_OR_NULL(new_fq)) {
+ if (IS_ERR(new_fq)) {
dev_err(qidev, "FQ allocation for shdesc update failed\n");
return PTR_ERR(new_fq);
}
@@ -431,7 +431,7 @@ struct caam_drv_ctx *caam_drv_ctx_init(struct device *qidev,
/* Attach request FQ */
drv_ctx->req_fq = create_caam_req_fq(qidev, drv_ctx->rsp_fq, hwdesc,
QMAN_INITFQ_FLAG_SCHED);
- if (IS_ERR_OR_NULL(drv_ctx->req_fq)) {
+ if (IS_ERR(drv_ctx->req_fq)) {
dev_err(qidev, "create_caam_req_fq failed\n");
dma_unmap_single(qidev, hwdesc, size, DMA_BIDIRECTIONAL);
kfree(drv_ctx);
--
2.17.1
Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static
On Wed, Feb 20, 2019 at 04:04:17PM +0530, Sumit Garg wrote: > On Wed, 20 Feb 2019 at 14:51, Wei Yongjun wrote: > > > > Fixes the following sparse warning: ^^ > > > > drivers/char/hw_random/optee-rng.c:265:35: warning: > > symbol 'optee_rng_id_table' was not declared. Should it be static? > > > > I haven't observed this warning during my normal Linux build using > gcc. Is there any specific configuration you are using? > It's from the Sparse tool. regards, dan carpenter
[PATCH] crypto: cavium/nitrox - Use after free in process_response_list()
We free "sr" and then dereference it on the next line.
Fixes: c9613335bf4f ("crypto: cavium/nitrox - Added AEAD cipher support")
Signed-off-by: Dan Carpenter
---
drivers/crypto/cavium/nitrox/nitrox_reqmgr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
b/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
index e34e4df8fd24..fe070d75c842 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_reqmgr.c
@@ -567,10 +567,10 @@ static void process_response_list(struct nitrox_cmdq
*cmdq)
/* ORH error code */
err = READ_ONCE(*sr->resp.orh) & 0xff;
- softreq_destroy(sr);
if (sr->callback)
sr->callback(sr->cb_arg, err);
+ softreq_destroy(sr);
req_completed++;
}
--
2.17.1
[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(&alg->encrypt_cnt);
43 raead.stat_encrypt_cnt = v32;
44 v64 = atomic64_read(&alg->encrypt_tlen);
45 raead.stat_encrypt_tlen = v64;
46 v32 = atomic_read(&alg->decrypt_cnt);
47 raead.stat_decrypt_cnt = v32;
48 v64 = atomic64_read(&alg->decrypt_tlen);
49 raead.stat_decrypt_tlen = v64;
50 v32 = atomic_read(&alg->aead_err_cnt);
51 raead.stat_aead_err_cnt = v32;
52
53 if (nla_put(skb, CRYPTOCFGA_STAT_AEAD,
54 sizeof(struct crypto_stat), &raead))
^^
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
Re: [PATCH 1/2] Fix static checker warning
Sounds reasonable. The subject needs a subsystem prefix and it might be better to make it more specific. Like so: [PATCH 1/2] crypto: ccp - potential uninitialized variable I don't know if Herbert fixes those things up himself normally? regards, dan carpenter
Re: [PATCH 3/7] vfio: add sdmdev support
Hi Kenneth,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.19-rc2 next-20180905]
[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/Kenneth-Lee/A-General-Accelerator-Framework-WarpDrive/20180903-162733
base:
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
smatch warnings:
drivers/vfio/sdmdev/vfio_sdmdev.c:78 iommu_type_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:91 dma_flag_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:127 flags_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:128 name_show() error: 'sdmdev' dereferencing
possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:130 device_api_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:138 available_instances_show() error:
'sdmdev' dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:178 vfio_sdmdev_mdev_remove() warn: if();
#
https://github.com/0day-ci/linux/commit/1e47d5e608652b4a2c813dbeaf5aa6811f6ceaf7
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 1e47d5e608652b4a2c813dbeaf5aa6811f6ceaf7
vim +/sdmdev +78 drivers/vfio/sdmdev/vfio_sdmdev.c
1e47d5e6 Kenneth Lee 2018-09-03 69
1e47d5e6 Kenneth Lee 2018-09-03 70 static ssize_t iommu_type_show(struct
device *dev,
1e47d5e6 Kenneth Lee 2018-09-03 71 struct
device_attribute *attr, char *buf)
1e47d5e6 Kenneth Lee 2018-09-03 72 {
1e47d5e6 Kenneth Lee 2018-09-03 73struct vfio_sdmdev *sdmdev =
vfio_sdmdev_pdev_sdmdev(dev);
^^^
Presumably this returns error pointers instead of NULL?
1e47d5e6 Kenneth Lee 2018-09-03 74
1e47d5e6 Kenneth Lee 2018-09-03 75if (!sdmdev)
1e47d5e6 Kenneth Lee 2018-09-03 76return -ENODEV;
1e47d5e6 Kenneth Lee 2018-09-03 77
1e47d5e6 Kenneth Lee 2018-09-03 @78return sprintf(buf, "%d\n",
sdmdev->iommu_type);
1e47d5e6 Kenneth Lee 2018-09-03 79 }
1e47d5e6 Kenneth Lee 2018-09-03 80
1e47d5e6 Kenneth Lee 2018-09-03 81 static DEVICE_ATTR_RO(iommu_type);
1e47d5e6 Kenneth Lee 2018-09-03 82
1e47d5e6 Kenneth Lee 2018-09-03 83 static ssize_t dma_flag_show(struct
device *dev,
1e47d5e6 Kenneth Lee 2018-09-03 84 struct
device_attribute *attr, char *buf)
1e47d5e6 Kenneth Lee 2018-09-03 85 {
1e47d5e6 Kenneth Lee 2018-09-03 86struct vfio_sdmdev *sdmdev =
vfio_sdmdev_pdev_sdmdev(dev);
1e47d5e6 Kenneth Lee 2018-09-03 87
1e47d5e6 Kenneth Lee 2018-09-03 88if (!sdmdev)
1e47d5e6 Kenneth Lee 2018-09-03 89return -ENODEV;
1e47d5e6 Kenneth Lee 2018-09-03 90
1e47d5e6 Kenneth Lee 2018-09-03 @91return sprintf(buf, "%d\n",
sdmdev->dma_flag);
1e47d5e6 Kenneth Lee 2018-09-03 92 }
1e47d5e6 Kenneth Lee 2018-09-03 93
1e47d5e6 Kenneth Lee 2018-09-03 94 static DEVICE_ATTR_RO(dma_flag);
1e47d5e6 Kenneth Lee 2018-09-03 95
1e47d5e6 Kenneth Lee 2018-09-03 96 /* mdev->dev_attr_groups */
1e47d5e6 Kenneth Lee 2018-09-03 97 static struct attribute
*vfio_sdmdev_attrs[] = {
1e47d5e6 Kenneth Lee 2018-09-03 98&dev_attr_iommu_type.attr,
1e47d5e6 Kenneth Lee 2018-09-03 99&dev_attr_dma_flag.attr,
1e47d5e6 Kenneth Lee 2018-09-03 100NULL,
1e47d5e6 Kenneth Lee 2018-09-03 101 };
1e47d5e6 Kenneth Lee 2018-09-03 102 static const struct attribute_group
vfio_sdmdev_group = {
1e47d5e6 Kenneth Lee 2018-09-03 103.name =
VFIO_SDMDEV_PDEV_ATTRS_GRP_NAME,
1e47d5e6 Kenneth Lee 2018-09-03 104.attrs = vfio_sdmdev_attrs,
1e47d5e6 Kenneth Lee 2018-09-03 105 };
1e47d5e6 Kenneth Lee 2018-09-03 106 const struct attribute_group
*vfio_sdmdev_groups[] = {
1e47d5e6 Kenneth Lee 2018-09-03 107&vfio_sdmdev_group,
1e47d5e6 Kenneth Lee 2018-09-03 108NULL,
1e47d5e6 Kenneth Lee 2018-09-03 109 };
1e47d5e6 Kenneth Lee 2018-09-03 110
1e47d5e6 Kenneth Lee 2018-09-03 111 /* default attributes for
mdev->supported_type_groups, used by registerer*/
1e47d5e6 Kenneth Lee 2018-09-03 112 #define MDEV_TYPE_ATTR_RO_EXPORT(name) \
1e47d5e6 Kenneth Lee 2018-09-03 113MDEV_TYPE_ATTR_RO(name); \
1e47d5e6 Kenneth Lee 2018-09-03 114
EXPORT_SYMBOL_GPL(mdev_type_attr_##name);
1e47d5e6 Kenneth Lee 2018-09-03 115
1e47d5e6 Kenneth Lee 2018-09-03 116 #define DEF_SIMPLE_SDMDEV_ATTR(_name,
sdmdev_member, format) \
1e47d5e6 Kenneth Lee 2018-09-03 117 static ssize_t _name##_show(struct
kobject *kobj, struct device *dev, \
1e47d5e6 Kenneth Lee 2018-09-03 118char *buf) \
1e47d5e6 Kenneth Lee 2018-09-03 119 { \
1e47d5e6 Kenneth Lee 2018-09-03 120
[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(&csk->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(&hdr,
&msg->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 = &dd->pdata->aead_algs_info->algs_list[i];
1241 algp = &aalg->base;
1242
1243 pr_debug("reg alg: %s\n", algp->cra_name);
1244 INIT_LIST_HEAD(&algp->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(&dev->kobj, &omap_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 = &dd->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(&queue->queuelock);
818
819 /*
820 * Can go on to queue if we have space in either:
821 * 1) The hardware queue and no software queue
822 * 2) The software queue
823 * AND there is nothing in the backlog. If there is backlog we
824 * have to only queue to the backlog queue and return busy.
825 */
826 if ((!sec_queue_can_enqueue(queue, steps) &&
827 (!queue->havesoftqueue ||
828kfifo_avail(&queue->softqueue) > steps)) ||
829 !list_empty(&ctx->backlog)) {
830 if ((skreq->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
831 list_add_tail(&sec_req->backlog_head,
&ctx->backlog);
832 mutex_unlock(&queue->queuelock);
833 return -EBUSY;
834 }
835
836 ret = -EBUSY;
837 mutex_unlock(&queue->queuelock);
838 goto err_free_elements;
^^
839 }
840 ret = sec_send_request(sec_req, queue);
841 mutex_unlock(&queue->queuelock);
842 if (ret)
843 goto err_free_elements;
^^
844
845 return -EINPROGRESS;
846
847 err_free_elements:
848 list_for_each_entry_safe(el, temp, &sec_req->elements, head) {
849 list_del(&el->head);
850 sec_alg_free_el(el, info);
851 }
852 if (crypto_skcipher_ivsize(atfm))
853 dma_unmap_single(info->dev, sec_req->dma_iv,
854 crypto_skcipher_ivsize(atfm),
855 DMA_BIDIRECTIONAL);
856 err_unmap_out_sg:
857 if (skreq->src != skreq->dst)
858 sec_unmap_sg_on_err(skreq->dst, steps, splits_out,
859 splits_out_nents, sec_req->len_out,
860 info->dev);
861 err_unmap_in_sg:
862 sec_unmap_sg_on_err(skreq->src, steps, splits_in,
splits_in_nents,
863 sec_req->len_in, info->dev);
864 err_free_split_sizes:
865 kfree(split_sizes);
^^^
Double free.
866
867 return ret;
868 }
regards,
dan carpenter
[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 = &psp_master->status_cmd_buf;
395 ret = sev_platform_status(status, &error);
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
> Signed-off-by: Atul Gupta
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), &wait);
> + 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->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->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->sk_socket->flags);
> + sk->sk_write_pending++;
> + sk_wait_event(sk, ¤t_timeo, sk->sk_err ||
> + (sk->sk_shutdown & SEND_SHUTDOWN) ||
> + (sndbuf > sk->sk_wmem_queued && !vm_wait), &wait);
(csk_mem_free(cdev, sk) && !vm_wait), &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), &wait);
> + 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(&tmp_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
*)&csk->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 20
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
> > Signed-off-by: Atul Gupta
> > ---
> > 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
> Signed-off-by: Atul Gupta
> ---
> 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
> Signed-off-by: Atul Gupta
> ---
> 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&csk->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, &crypto_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, &timeo, 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->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->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 = &dd->pdata->aead_algs_info->algs_list[i];
1241 algp = &aalg->base;
1242
1243 pr_debug("reg alg: %s\n", algp->cra_name);
1244 INIT_LIST_HEAD(&algp->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(&dev->kobj, &omap_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 = &dd->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 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(&error);
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
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 = &tmpl->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",
- &dma_mask);
+ dev_err(dev, "Failed in dma_set_mask, mask=%par\n", &dma_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(&fips_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,
-&larval_seq_len);
+ cc_set_sram_desc(md5_init, sram_buff_ofs, ARRAY_SIZE(md5_init),
+larval_seq, &larval_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 c
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 &dma_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
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 > 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 > 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
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)
&dd->hash_flags)) {
/* hash or semi-hash ready */
clear_bit(HASH_FLAGS_DMA_READY, &dd->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
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 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
> 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(&desc[idx]);
> set_din_const(&desc[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, &ssi_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
> ---
> 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, > - &areq_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, > + &areq_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",
> + &dma_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(&plat_dev->dev, dma_mask)) {
> + rc = dma_set_coherent_mask(&plat_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
> ---
> 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(&areq_ctx->ccm_adata_sg, config_data, AES_BLOCK_SIZE +
> areq_ctx->ccm_hdr_size);
> if (unlikely(dma_map_sg(dev, &areq_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",
> &sg_dma_address(&areq_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",
> &sg_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
> ---
> 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
> ---
> 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] Staging: ccree: ssi_cipher.c: Correct spelling mistake.
On Thu, Sep 07, 2017 at 12:54:23AM +0530, Srishti Sharma wrote: > Correct spelling of counter in comment . > > Signed-off-by: Srishti Sharma > --- > drivers/staging/ccree/ssi_cipher.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/ccree/ssi_cipher.c > b/drivers/staging/ccree/ssi_cipher.c > index 8d31a93..99232b2 100644 > --- a/drivers/staging/ccree/ssi_cipher.c > +++ b/drivers/staging/ccree/ssi_cipher.c > @@ -702,7 +702,7 @@ static int ssi_blkcipher_complete(struct device *dev, > > ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst); > > - /*Set the inflight couter value to local variable*/ > + /*Set the inflight counter value to local variable*/ > inflight_counter = ctx_p->drvdata->inflight_counter; Sure, but it would be better to just delete the comment. It's obvious. But really just delete the local inflight_counter variable as well because that's never used. 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(&plat_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(&plat_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(&aead_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(&hash_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 = &work_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,
&cmd->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,
&istate, &ostate);
883 if (ret)
884 return ret;
885
886 memcpy(ctx->ipad, &istate.state, SHA1_DIGEST_SIZE);
^
887 memcpy(ctx->opad, &ostate.state, 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 = &ctx->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, &tmp, 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
> ---
> 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 > 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
>
> Fixed "ERROR: space prohibited after that open parenthesis '('".
>
> Signed-off-by: Jhih-Ming Hunag
> ---
> 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 > > 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 > 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
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, &req->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, &sr->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, &sr->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
---
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
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;
