Re: [PATCH RESEND v2 00/14] lib/mpi: bug fixes and cleanup
Hi Nicolai,
On 03/21/2016 06:26 AM, Nicolai Stange wrote:
> This is a resend of v2 with the crypto people properly CC'd.
>
> The original v1 can be found here:
>
>
> http://lkml.kernel.org/g/1458237606-4954-1-git-send-email-nicsta...@gmail.com
>
>
> While v1 (hopefully) fixed some issues in mpi_write_sgl() and
> mpi_read_buffer() introduced by
> commit 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") and by
> commit 9cbe21d8f89d ("lib/mpi: only require buffers as big as needed for
> the integer"),
> I missed that there are some, including out-of-bounds buffer accesses,
> in mpi_read_raw_from_sgl() as well.
>
> Hence v2, which includes the original stuff from v1 plus my new fixes to
> mpi_read_raw_from_sgl().
>
>
> Applicable to linux-next-20160318.
>
>
> Changes to v1:
> - [1-8/14]
> former [1-8/8], unchanged.
>
> - [9-14/14]
> Added in v2. Fixes to mpi_read_raw_from_sgl().
>
> Nicolai Stange (14):
> lib/mpi: mpi_write_sgl(): fix skipping of leading zero limbs
> lib/mpi: mpi_write_sgl(): fix style issue with lzero decrement
> lib/mpi: mpi_write_sgl(): purge redundant pointer arithmetic
> lib/mpi: mpi_write_sgl(): fix out-of-bounds stack access
> lib/mpi: mpi_write_sgl(): replace open coded endian conversion
> lib/mpi: mpi_read_buffer(): optimize skipping of leading zero limbs
> lib/mpi: mpi_read_buffer(): replace open coded endian conversion
> lib/mpi: mpi_read_buffer(): fix buffer overflow
> lib/mpi: mpi_read_raw_from_sgl(): replace len argument by nbytes
> lib/mpi: mpi_read_raw_from_sgl(): don't include leading zero SGEs in
> nbytes
> lib/mpi: mpi_read_raw_from_sgl(): purge redundant clearing of nbits
> lib/mpi: mpi_read_raw_from_sgl(): fix nbits calculation
> lib/mpi: mpi_read_raw_from_sgl(): sanitize meaning of indices
> lib/mpi: mpi_read_raw_from_sgl(): fix out-of-bounds buffer access
>
> lib/mpi/mpicoder.c | 122
> +++--
> 1 file changed, 43 insertions(+), 79 deletions(-)
Thanks for sending this. Nice work. In fact the mpi_write_sgl() function
worked only because the mpi_normalize() stripped all MSB zero limbs.
Given that this will hold for all cases we can simplify this even more.
Unfortunately I don't know if this will be true for mpi_sub() or
mpi_set_ui() because they are declared in include/linux/mpi.h,
but never defined anywhere.
I've found one problem in 08/14 in mpi_read_buffer()
It's a pointer arithmetic issue, which causes the first limb to be
written at an invalid address if lzeros > 0. This incremental patch
fixes it.
---8<---
Subject: [PATCH] lib/mpi: fix pointer arithmetic issue in mpi_read_buffer
Fix pointer arithmetic issue, which causes the first limb to be
written at invalid address if lzeros > 0.
Signed-off-by: Tadeusz Struk
---
lib/mpi/mpicoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 0c534ac..747606f 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -201,7 +201,7 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len,
unsigned *nbytes,
#else
#error please implement for this limb size.
#endif
- memcpy(p, &alimb + lzeros, BYTES_PER_MPI_LIMB - lzeros);
+ memcpy(p, (u8 *)&alimb + lzeros, BYTES_PER_MPI_LIMB - lzeros);
p += BYTES_PER_MPI_LIMB - lzeros;
lzeros = 0;
}
---
Other than that please include my
Tested-by: Tadeusz Struk
for the whole series.
Thanks,
--
TS
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: Avoid undefined behavior in macro expansion
On Fri, Mar 18, 2016 at 11:15 PM, Al Viro wrote: > On Wed, Mar 16, 2016 at 11:48:49PM -0300, Vinicius Tinti wrote: >> C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined >> behavior when one of the result operands is not a valid preprocessing >> token. >> >> Therefore the macro expansion may depend on compiler implementation >> which may or no preserve the leading white space. >> >> Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). >> Changing favors concise usage. > > Huh? > >> -#define XMM(i) CONCAT(%xmm, i) >> +#define XMM(i) CONCAT(%xmm,i) > > What are you talking about? Undefined behaviour is when the result of > concatenation of adjacent tokens is not a valid preprocessor token. > It says nothing about the either argument being a single token. Please check the example below otherwise it will be hard to explain. The problem is that _i_ can be a macro to be expanded too. And it can be a parameter for a _paste_ operator. // tricky code #define CONCAT(a,b)a##b #defineXMM(i)CONCAT(%xmm, i) .macro foo n x = XMM(\n) .endm _%xmm_ is not a problem but _i_ is. > In this case after the substitution of e.g. XMM(42) we get 3 tokens: > Punctuator[%] Identifier[xmm] Pp-number[42] > with ## instructing us to replace the last two with preprocessor token that > would be represented as concatenation of their representations. Which is > to say, concatenation of xmm and 42, i.e. xmm42. Which *is* a > representation of a valid preprocessor token - namely, Identifier[xmm42]. Agree. But it is not this case. I will add the code above at commit and describe it. It will be easy to explain what I am trying to solve. > No undefined behaviour at all. And yes, you get two preprocessor tokens > in the expansion - % and xmm42. Preprocessor works in terms of tokens, > not strings... Understood. > If you know of any compiler where these two variants would produce different > expansions of XMM(), please report it to maintainers of > the compiler in question; it's a bug, plain and simple. And no, there's > no undefined behaviour in that. I reported a bug and discussed over it and I too believe that the tricky code that I have just sent triggers an undefined behavior. What do you think? -- Simplicity is the ultimate sophistication -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] crypto: s5p-sss - Sort the headers to improve readability
Sort the headers alphabetically to improve readability and to spot duplications easier. Suggested-by: Vladimir Zapolskiy Signed-off-by: Krzysztof Kozlowski Acked-by: Vladimir Zapolskiy --- Changes since v2: 1. Add Vladimir's acked-by. Changes since v1: 1. New patch. My email differs from one used in previous patches because this time I am sending this from work. --- drivers/crypto/s5p-sss.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index 3730fb0af4d8..4f6d5b3ec418 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -11,23 +11,23 @@ * */ +#include +#include +#include #include -#include -#include #include +#include +#include +#include #include -#include +#include +#include #include #include -#include -#include -#include -#include -#include -#include -#include #include +#include +#include #include #define _SBF(s, v) ((v) << (s)) -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] crypto: s5p-sss - Minor coding cleanups
From: Krzysztof Kozlowski Remove unneeded inclusion of delay.h and get rid of indentation from labels. Signed-off-by: Krzysztof Kozlowski Acked-by: Vladimir Zapolskiy --- Changes since v2: 1. None. Changes since v1: 1. Add Vladimir's acked-by. --- drivers/crypto/s5p-sss.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index 5f161a9777e3..60f835455a41 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -11,7 +11,6 @@ * */ -#include #include #include #include @@ -284,7 +283,7 @@ static int s5p_set_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg) dev->sg_dst = sg; err = 0; - exit: +exit: return err; } @@ -310,7 +309,7 @@ static int s5p_set_indata(struct s5p_aes_dev *dev, struct scatterlist *sg) dev->sg_src = sg; err = 0; - exit: +exit: return err; } @@ -452,10 +451,10 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode) return; - outdata_error: +outdata_error: s5p_unset_indata(dev); - indata_error: +indata_error: s5p_aes_complete(dev, err); spin_unlock_irqrestore(&dev->lock, flags); } @@ -506,7 +505,7 @@ static int s5p_aes_handle_req(struct s5p_aes_dev *dev, tasklet_schedule(&dev->tasklet); - exit: +exit: return err; } @@ -705,7 +704,7 @@ static int s5p_aes_probe(struct platform_device *pdev) return 0; - err_algs: +err_algs: dev_err(dev, "can't register '%s': %d\n", algs[i].cra_name, err); for (j = 0; j < i; j++) @@ -713,7 +712,7 @@ static int s5p_aes_probe(struct platform_device *pdev) tasklet_kill(&pdata->tasklet); - err_irq: +err_irq: clk_disable_unprepare(pdata->clk); s5p_dev = NULL; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] crypto: s5p-sss - Handle unaligned buffers
From: Krzysztof Kozlowski
During crypto selftests on Odroid XU3 (Exynos5422) some of the
algorithms failed because of passing AES-block unaligned source and
destination buffers:
alg: skcipher: encryption failed on chunk test 1 for ecb-aes-s5p: ret=22
Handle such case by copying the buffers to a new aligned and contiguous
space.
Signed-off-by: Krzysztof Kozlowski
Acked-by: Vladimir Zapolskiy
---
Changes since v2:
1. Fix infinite loop in s5p_is_sg_aligned().
Changes since v1:
1. Add Vladimir's acked-by.
Driver tested on Exynos4412/Trats2 and Exynos5422/Odroid XU4 with crypto
manager self-tests and tcrypt:
$ modprobe tcrypt sec=1 mode=500
---
drivers/crypto/s5p-sss.c | 150 +++
1 file changed, 138 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 60f835455a41..3730fb0af4d8 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#define _SBF(s, v) ((v) << (s))
#define _BIT(b) _SBF(b, 1)
@@ -185,6 +186,10 @@ struct s5p_aes_dev {
struct scatterlist *sg_src;
struct scatterlist *sg_dst;
+ /* In case of unaligned access: */
+ struct scatterlist *sg_src_cpy;
+ struct scatterlist *sg_dst_cpy;
+
struct tasklet_struct tasklet;
struct crypto_queue queue;
boolbusy;
@@ -244,8 +249,45 @@ static void s5p_set_dma_outdata(struct s5p_aes_dev *dev,
struct scatterlist *sg)
SSS_WRITE(dev, FCBTDMAL, sg_dma_len(sg));
}
+static void s5p_free_sg_cpy(struct s5p_aes_dev *dev, struct scatterlist **sg)
+{
+ int len;
+
+ if (!*sg)
+ return;
+
+ len = ALIGN(dev->req->nbytes, AES_BLOCK_SIZE);
+ free_pages((unsigned long)sg_virt(*sg), get_order(len));
+
+ kfree(*sg);
+ *sg = NULL;
+}
+
+static void s5p_sg_copy_buf(void *buf, struct scatterlist *sg,
+ unsigned int nbytes, int out)
+{
+ struct scatter_walk walk;
+
+ if (!nbytes)
+ return;
+
+ scatterwalk_start(&walk, sg);
+ scatterwalk_copychunks(buf, &walk, nbytes, out);
+ scatterwalk_done(&walk, out, 0);
+}
+
static void s5p_aes_complete(struct s5p_aes_dev *dev, int err)
{
+ if (dev->sg_dst_cpy) {
+ dev_dbg(dev->dev,
+ "Copying %d bytes of output data back to original
place\n",
+ dev->req->nbytes);
+ s5p_sg_copy_buf(sg_virt(dev->sg_dst_cpy), dev->req->dst,
+ dev->req->nbytes, 1);
+ }
+ s5p_free_sg_cpy(dev, &dev->sg_src_cpy);
+ s5p_free_sg_cpy(dev, &dev->sg_dst_cpy);
+
/* holding a lock outside */
dev->req->base.complete(&dev->req->base, err);
dev->busy = false;
@@ -261,14 +303,36 @@ static void s5p_unset_indata(struct s5p_aes_dev *dev)
dma_unmap_sg(dev->dev, dev->sg_src, 1, DMA_TO_DEVICE);
}
+static int s5p_make_sg_cpy(struct s5p_aes_dev *dev, struct scatterlist *src,
+ struct scatterlist **dst)
+{
+ void *pages;
+ int len;
+
+ *dst = kmalloc(sizeof(**dst), GFP_ATOMIC);
+ if (!*dst)
+ return -ENOMEM;
+
+ len = ALIGN(dev->req->nbytes, AES_BLOCK_SIZE);
+ pages = (void *)__get_free_pages(GFP_ATOMIC, get_order(len));
+ if (!pages) {
+ kfree(*dst);
+ *dst = NULL;
+ return -ENOMEM;
+ }
+
+ s5p_sg_copy_buf(pages, src, dev->req->nbytes, 0);
+
+ sg_init_table(*dst, 1);
+ sg_set_buf(*dst, pages, len);
+
+ return 0;
+}
+
static int s5p_set_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg)
{
int err;
- if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE)) {
- err = -EINVAL;
- goto exit;
- }
if (!sg_dma_len(sg)) {
err = -EINVAL;
goto exit;
@@ -291,10 +355,6 @@ static int s5p_set_indata(struct s5p_aes_dev *dev, struct
scatterlist *sg)
{
int err;
- if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE)) {
- err = -EINVAL;
- goto exit;
- }
if (!sg_dma_len(sg)) {
err = -EINVAL;
goto exit;
@@ -394,6 +454,71 @@ static void s5p_set_aes(struct s5p_aes_dev *dev,
memcpy_toio(keystart, key, keylen);
}
+static bool s5p_is_sg_aligned(struct scatterlist *sg)
+{
+ while (sg) {
+ if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE))
+ return false;
+ sg = sg_next(sg);
+ }
+
+ return true;
+}
+
+static int s5p_set_indata_start(struct s5p_aes_dev *dev,
+ struct ablkcipher_request *req)
+{
+ struct scatterlist *sg;
+ int err;
+
+
Re: [PATCH 02/10] crypto: rsa_helper - add raw integer parser actions
Am Montag, 21. März 2016, 15:17:20 schrieb Tudor-Dan Ambarus:
Hi Tudor,
> Hi Stephan,
>
> > -Original Message-
> > From: Stephan Mueller [mailto:smuel...@chronox.de]
> > Sent: Friday, March 18, 2016 9:47 PM
> > To: Tudor-Dan Ambarus
> > Cc: herb...@gondor.apana.org.au; tadeusz.st...@intel.com; linux-
> > cry...@vger.kernel.org; Horia Ioan Geanta Neag
> > Subject: Re: [PATCH 02/10] crypto: rsa_helper - add raw integer parser
> > actions
> >
> > > +int rsa_check_key_length(unsigned int len)
> > > +{
> > > + switch (len) {
> > > + case 512:
> > > + case 1024:
> > > + case 1536:
> > > + case 2048:
> > > + case 3072:
> > > + case 4096:
> > > + return 0;
> > > + }
> >
> > I know that you copied the code to a new location that was there already.
> > But
> > based on the discussion we had for DH, does it make sense that the kernel
> > adds
> > such (artificial) limits?
>
> [ta] This is not within the scope of this patch set, but we can remove the
> restrictions in a subsequent patch. Marcel has suggested to not impose
> limits on the minimum length of the key. What about the maximum?
Why any restrictions at all? There is no mathematical reason. There is only a
technical reason which depends on the implementation.
I am not sure how large the keys can be for the software fallback. But maybe
it would make sense to have a general cap like 32k where the software fallback
can handle all key sizes up to that point.
> > > +
> > > + return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(rsa_check_key_length);
> > > +
> > > +int raw_rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
> > > + const void *value, size_t vlen)
> > > +{
> > > + struct rsa_raw_ctx *ctx = context;
> > > + struct rsa_raw_key *key = &ctx->key;
> > > + const char *ptr = value;
> > > + int ret = -EINVAL;
> > > +
> > > + while (!*ptr && vlen) {
> > > + ptr++;
> > > + vlen--;
> > > + }
> > > +
> > > + key->n_sz = vlen;
> > > + /* In FIPS mode only allow key size 2K & 3K */
> > > + if (fips_enabled && (key->n_sz != 256 && key->n_sz != 384)) {
> >
> > Again, you copied that code that used to be there . But very very
> > recently,
> > NIST allowed 4k keys too. May I ask to allow it here?
>
> I suggest to do this in a separate patch. Can you send us a pointer to the
> NIST specification?
Sure, just let us not forget about it.
>
> Thank you,
> ta
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
FOR YOUR INFORMATION DEAR BENEFICIARY,
FOR YOUR INFORMATION DEAR BENEFICIARY, Your Over-due ATM Card payment of $1.5MUSD by the UN Office have deposited to the ATM MASTER CARD OFFICE. All you have to do now is to contact the Office Manager Dr. peter Akupa Boni at: (peterakupa...@gmail.com) and Phone number: +229 61 31 07 78 , he will give you direction on how you will receive your ATM MASTER CARD immediately. Contact person: Dr. peter Akupa, E-mail: (peterakupa...@gmail.com ) You have to also contact him with the following details: 1. Your full name: 2. Your contact cell phone number: 3. Your age: 4. your Email Thanks David Kedogo -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] MAINTAINERS: Add a new maintainer for the CCP driver
Gary will be taking over future development of the CCP driver, so add him as a co-maintainer of the driver. Signed-off-by: Tom Lendacky --- MAINTAINERS |1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 30aca4a..8c42c07 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -623,6 +623,7 @@ F: include/linux/altera_jtaguart.h AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER M: Tom Lendacky +M: Gary Hook L: linux-crypto@vger.kernel.org S: Supported F: drivers/crypto/ccp/ -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/10] crypto: rsa - generalize ASN.1 sequences
On 03/21/2016 03:11 AM, Tudor-Dan Ambarus wrote: > [ta] we can do this once, when initializing the tfm object. > > I agree with all your suggestions, I will implement them. > > Thank you for the review, Great. Thank you Tudor. -- TS -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] crypto: rsa_helper - add raw integer parser actions
Hi Stephan,
> -Original Message-
> From: Stephan Mueller [mailto:smuel...@chronox.de]
> Sent: Friday, March 18, 2016 9:47 PM
> To: Tudor-Dan Ambarus
> Cc: herb...@gondor.apana.org.au; tadeusz.st...@intel.com; linux-
> cry...@vger.kernel.org; Horia Ioan Geanta Neag
> Subject: Re: [PATCH 02/10] crypto: rsa_helper - add raw integer parser
> actions
>
> > +int rsa_check_key_length(unsigned int len)
> > +{
> > + switch (len) {
> > + case 512:
> > + case 1024:
> > + case 1536:
> > + case 2048:
> > + case 3072:
> > + case 4096:
> > + return 0;
> > + }
>
> I know that you copied the code to a new location that was there already.
> But
> based on the discussion we had for DH, does it make sense that the kernel
> adds
> such (artificial) limits?
[ta] This is not within the scope of this patch set, but we can remove the
restrictions in a subsequent patch. Marcel has suggested to not impose limits
on the minimum length of the key. What about the maximum?
> > +
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(rsa_check_key_length);
> > +
> > +int raw_rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
> > + const void *value, size_t vlen)
> > +{
> > + struct rsa_raw_ctx *ctx = context;
> > + struct rsa_raw_key *key = &ctx->key;
> > + const char *ptr = value;
> > + int ret = -EINVAL;
> > +
> > + while (!*ptr && vlen) {
> > + ptr++;
> > + vlen--;
> > + }
> > +
> > + key->n_sz = vlen;
> > + /* In FIPS mode only allow key size 2K & 3K */
> > + if (fips_enabled && (key->n_sz != 256 && key->n_sz != 384)) {
>
> Again, you copied that code that used to be there . But very very recently,
> NIST allowed 4k keys too. May I ask to allow it here?
>
I suggest to do this in a separate patch. Can you send us a pointer to the NIST
specification?
Thank you,
ta
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 04/14] lib/mpi: mpi_write_sgl(): fix out-of-bounds stack access
Within the copying loop in mpi_write_sgl(), we have
if (lzeros) {
mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
+ lzeros;
*limb1 = *limb2;
...
}
where p points past the end of alimb2 which lives on the stack and contains
the current limb in BE order.
The purpose of the above is to shift the non-zero bytes of alimb2 to its
beginning in memory, i.e. to skip its leading zero bytes.
However, limb2 points somewhere into the middle of alimb2 and thus, reading
*limb2 pulls in lzero bytes from somewhere.
Indeed, KASAN splats:
BUG: KASAN: stack-out-of-bounds in mpi_write_to_sgl+0x4e3/0x6f0
at addr 8800cb04f601
Read of size 8 by task systemd-udevd/391
page:ea00032c13c0 count:0 mapcount:0 mapping: (null) index:0x0
flags: 0x3fff80()
page dumped because: kasan: bad access detected
CPU: 3 PID: 391 Comm: systemd-udevd Tainted: G B L
4.5.0-next-20160316+ #12
[...]
Call Trace:
[] dump_stack+0xdc/0x15e
[] ? _atomic_dec_and_lock+0xa2/0xa2
[] ? __dump_page+0x185/0x330
[] kasan_report_error+0x5e6/0x8b0
[] ? kzfree+0x2d/0x40
[] ? mpi_free_limb_space+0xe/0x20
[] ? mpi_powm+0x37e/0x16f0
[] kasan_report+0x71/0xa0
[] ? mpi_write_to_sgl+0x4e3/0x6f0
[] __asan_load8+0x64/0x70
[] mpi_write_to_sgl+0x4e3/0x6f0
[] ? mpi_set_buffer+0x620/0x620
[] ? mpi_cmp+0xbf/0x180
[] rsa_verify+0x202/0x260
What's more, since lzeros can be anything from 1 to sizeof(mpi_limb_t)-1,
the above will cause unaligned accesses which is bad on non-x86 archs.
Fix the issue, by preparing the starting point p for the upcoming copy
operation instead of shifting the source memory, i.e. alimb2.
Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 78ec4e1..b05d390 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -403,15 +403,11 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
#error please implement for this limb size.
#endif
if (lzeros) {
- mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
- mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
- + lzeros;
- *limb1 = *limb2;
y = lzeros;
lzeros = 0;
}
- p = p - sizeof(alimb);
+ p = p - sizeof(alimb) + y;
for (x = 0; x < sizeof(alimb) - y; x++) {
if (!buf_len) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 01/14] lib/mpi: mpi_write_sgl(): fix skipping of leading zero limbs
Currently, if the number of leading zeros is greater than fits into a
complete limb, mpi_write_sgl() skips them by iterating over them limb-wise.
However, it fails to adjust its internal leading zeros tracking variable,
lzeros, accordingly: it does a
p -= sizeof(alimb);
continue;
which should really have been a
lzeros -= sizeof(alimb);
continue;
Since lzeros never decreases if its initial value >= sizeof(alimb), nothing
gets copied by mpi_write_sgl() in that case.
Instead of skipping the high order zero limbs within the loop as shown
above, fix the issue by adjusting the copying loop's bounds.
Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index eb15e7d..6bb52be 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -380,7 +380,9 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
buf_len = sgl->length;
p2 = sg_virt(sgl);
- for (i = a->nlimbs - 1; i >= 0; i--) {
+ for (i = a->nlimbs - 1 - lzeros / BYTES_PER_MPI_LIMB,
+ lzeros %= BYTES_PER_MPI_LIMB;
+ i >= 0; i--) {
alimb = a->d[i];
p = (u8 *)&alimb2;
#if BYTES_PER_MPI_LIMB == 4
@@ -401,17 +403,12 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
#error please implement for this limb size.
#endif
if (lzeros > 0) {
- if (lzeros >= sizeof(alimb)) {
- p -= sizeof(alimb);
- continue;
- } else {
- mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
- mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
- + lzeros;
- *limb1 = *limb2;
- p -= lzeros;
- y = lzeros;
- }
+ mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
+ mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
+ + lzeros;
+ *limb1 = *limb2;
+ p -= lzeros;
+ y = lzeros;
lzeros -= sizeof(alimb);
}
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 02/14] lib/mpi: mpi_write_sgl(): fix style issue with lzero decrement
Within the copying loop in mpi_write_sgl(), we have
if (lzeros > 0) {
...
lzeros -= sizeof(alimb);
}
However, at this point, lzeros < sizeof(alimb) holds. Make this fact
explicit by rewriting the above to
if (lzeros) {
...
lzeros = 0;
}
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 6bb52be..d8b372b 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -402,14 +402,14 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
#else
#error please implement for this limb size.
#endif
- if (lzeros > 0) {
+ if (lzeros) {
mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
+ lzeros;
*limb1 = *limb2;
p -= lzeros;
y = lzeros;
- lzeros -= sizeof(alimb);
+ lzeros = 0;
}
p = p - (sizeof(alimb) - y);
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 03/14] lib/mpi: mpi_write_sgl(): purge redundant pointer arithmetic
Within the copying loop in mpi_write_sgl(), we have
if (lzeros) {
...
p -= lzeros;
y = lzeros;
}
p = p - (sizeof(alimb) - y);
If lzeros == 0, then y == 0, too. Thus, lzeros gets subtracted and added
back again to p.
Purge this redundancy.
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index d8b372b..78ec4e1 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -407,12 +407,11 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
+ lzeros;
*limb1 = *limb2;
- p -= lzeros;
y = lzeros;
lzeros = 0;
}
- p = p - (sizeof(alimb) - y);
+ p = p - sizeof(alimb);
for (x = 0; x < sizeof(alimb) - y; x++) {
if (!buf_len) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 07/14] lib/mpi: mpi_read_buffer(): replace open coded endian conversion
Currently, the endian conversion from CPU order to BE is open coded in
mpi_read_buffer().
Replace this by the centrally provided cpu_to_be*() macros.
Copy from the temporary storage on stack to the destination buffer
by means of memcpy().
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 27 ---
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 2fd8d41..a999ee1 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include "mpi-internal.h"
#define MAX_EXTERN_MPI_BITS 16384
@@ -164,7 +165,13 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len,
unsigned *nbytes,
int *sign)
{
uint8_t *p;
- mpi_limb_t alimb;
+#if BYTES_PER_MPI_LIMB == 4
+ __be32 alimb;
+#elif BYTES_PER_MPI_LIMB == 8
+ __be64 alimb;
+#else
+#error please implement for this limb size.
+#endif
unsigned int n = mpi_get_size(a);
int i, lzeros;
@@ -187,25 +194,15 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned
buf_len, unsigned *nbytes,
for (i = a->nlimbs - 1 - lzeros / BYTES_PER_MPI_LIMB,
lzeros %= BYTES_PER_MPI_LIMB;
i >= 0; i--) {
- alimb = a->d[i];
#if BYTES_PER_MPI_LIMB == 4
- *p++ = alimb >> 24;
- *p++ = alimb >> 16;
- *p++ = alimb >> 8;
- *p++ = alimb;
+ alimb = cpu_to_be32(a->d[i]);
#elif BYTES_PER_MPI_LIMB == 8
- *p++ = alimb >> 56;
- *p++ = alimb >> 48;
- *p++ = alimb >> 40;
- *p++ = alimb >> 32;
- *p++ = alimb >> 24;
- *p++ = alimb >> 16;
- *p++ = alimb >> 8;
- *p++ = alimb;
+ alimb = cpu_to_be64(a->d[i]);
#else
#error please implement for this limb size.
#endif
-
+ memcpy(p, &alimb, BYTES_PER_MPI_LIMB);
+ p += BYTES_PER_MPI_LIMB;
if (lzeros > 0) {
mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 14/14] lib/mpi: mpi_read_raw_from_sgl(): fix out-of-bounds buffer access
Within the copying loop in mpi_read_raw_from_sgl(), the last input SGE's
byte count gets artificially extended as follows:
if (sg_is_last(sg) && (len % BYTES_PER_MPI_LIMB))
len += BYTES_PER_MPI_LIMB - (len % BYTES_PER_MPI_LIMB);
Within the following byte copying loop, this causes reads beyond that
SGE's allocated buffer:
BUG: KASAN: slab-out-of-bounds in mpi_read_raw_from_sgl+0x331/0x650
at addr 8801e168d4d8
Read of size 1 by task systemd-udevd/721
[...]
Call Trace:
[] dump_stack+0xbc/0x117
[] ? _atomic_dec_and_lock+0x169/0x169
[] ? print_section+0x61/0xb0
[] print_trailer+0x179/0x2c0
[] object_err+0x34/0x40
[] kasan_report_error+0x307/0x8c0
[] ? kasan_unpoison_shadow+0x35/0x50
[] ? kasan_kmalloc+0x5e/0x70
[] kasan_report+0x71/0xa0
[] ? mpi_read_raw_from_sgl+0x331/0x650
[] __asan_load1+0x46/0x50
[] mpi_read_raw_from_sgl+0x331/0x650
[] rsa_verify+0x106/0x260
[] ? rsa_set_pub_key+0xf0/0xf0
[] ? sg_init_table+0x29/0x50
[] ? pkcs1pad_sg_set_buf+0xb2/0x2e0
[] pkcs1pad_verify+0x1f4/0x2b0
[] public_key_verify_signature+0x3a7/0x5e0
[] ? public_key_describe+0x80/0x80
[] ? keyring_search_aux+0x150/0x150
[] ? x509_request_asymmetric_key+0x114/0x370
[] ? kfree+0x220/0x370
[] public_key_verify_signature_2+0x32/0x50
[] verify_signature+0x7c/0xb0
[] pkcs7_validate_trust+0x42c/0x5f0
[] system_verify_data+0xca/0x170
[] ? top_trace_array+0x9b/0x9b
[] ? __vfs_read+0x279/0x3d0
[] mod_verify_sig+0x1ff/0x290
[...]
The exact purpose of the len extension isn't clear to me, but due to
its form, I suspect that it's a leftover somehow accounting for leading
zero bytes within the most significant output limb.
Note however that without that len adjustement, the total number of bytes
ever processed by the inner loop equals nbytes and thus, the last output
limb gets written at this point. Thus the net effect of the len adjustement
cited above is just to keep the inner loop running for some more
iterations, namely < BYTES_PER_MPI_LIMB ones, reading some extra bytes from
beyond the last SGE's buffer and discarding them afterwards.
Fix this issue by purging the extension of len beyond the last input SGE's
buffer length.
Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 3f114d2..0c534ac 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -484,9 +484,6 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned
int nbytes)
const u8 *buffer = sg_virt(sg) + lzeros;
int len = sg->length - lzeros;
- if (sg_is_last(sg) && (len % BYTES_PER_MPI_LIMB))
- len += BYTES_PER_MPI_LIMB - (len % BYTES_PER_MPI_LIMB);
-
for (x = 0; x < len; x++) {
a <<= 8;
a |= *buffer++;
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 11/14] lib/mpi: mpi_read_raw_from_sgl(): purge redundant clearing of nbits
In mpi_read_raw_from_sgl(), unsigned nbits is calculated as follows: nbits = nbytes * 8; and redundantly cleared later on if nbytes == 0: if (nbytes > 0) ... else nbits = 0; Purge this redundant clearing for the sake of clarity. Signed-off-by: Nicolai Stange --- lib/mpi/mpicoder.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c index add9e81..4cf8516 100644 --- a/lib/mpi/mpicoder.c +++ b/lib/mpi/mpicoder.c @@ -461,8 +461,6 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes) if (nbytes > 0) nbits -= count_leading_zeros(*(u8 *)(sg_virt(sgl) + lzeros)); - else - nbits = 0; nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB); val = mpi_alloc(nlimbs); -- 2.7.3 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 05/14] lib/mpi: mpi_write_sgl(): replace open coded endian conversion
Currently, the endian conversion from CPU order to BE is open coded in
mpi_write_sgl().
Replace this by the centrally provided cpu_to_be*() macros.
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 27 +++
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index b05d390..623439e 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -20,6 +20,7 @@
#include
#include
+#include
#include "mpi-internal.h"
#define MAX_EXTERN_MPI_BITS 16384
@@ -359,7 +360,13 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
int *sign)
{
u8 *p, *p2;
- mpi_limb_t alimb, alimb2;
+#if BYTES_PER_MPI_LIMB == 4
+ __be32 alimb;
+#elif BYTES_PER_MPI_LIMB == 8
+ __be64 alimb;
+#else
+#error please implement for this limb size.
+#endif
unsigned int n = mpi_get_size(a);
int i, x, y = 0, lzeros, buf_len;
@@ -383,22 +390,10 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
for (i = a->nlimbs - 1 - lzeros / BYTES_PER_MPI_LIMB,
lzeros %= BYTES_PER_MPI_LIMB;
i >= 0; i--) {
- alimb = a->d[i];
- p = (u8 *)&alimb2;
#if BYTES_PER_MPI_LIMB == 4
- *p++ = alimb >> 24;
- *p++ = alimb >> 16;
- *p++ = alimb >> 8;
- *p++ = alimb;
+ alimb = cpu_to_be32(a->d[i]);
#elif BYTES_PER_MPI_LIMB == 8
- *p++ = alimb >> 56;
- *p++ = alimb >> 48;
- *p++ = alimb >> 40;
- *p++ = alimb >> 32;
- *p++ = alimb >> 24;
- *p++ = alimb >> 16;
- *p++ = alimb >> 8;
- *p++ = alimb;
+ alimb = cpu_to_be64(a->d[i]);
#else
#error please implement for this limb size.
#endif
@@ -407,7 +402,7 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl,
unsigned *nbytes,
lzeros = 0;
}
- p = p - sizeof(alimb) + y;
+ p = (u8 *)&alimb + y;
for (x = 0; x < sizeof(alimb) - y; x++) {
if (!buf_len) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 13/14] lib/mpi: mpi_read_raw_from_sgl(): sanitize meaning of indices
Within the byte reading loop in mpi_read_raw_sgl(), there are two
housekeeping indices used, z and x.
At all times, the index z represents the number of output bytes covered
by the input SGEs for which processing has completed so far. This includes
any leading zero bytes within the most significant limb.
The index x changes its meaning after the first outer loop's iteration
though: while processing the first input SGE, it represents
"number of leading zero bytes in most significant output limb" +
"current position within current SGE"
For the remaining SGEs OTOH, x corresponds just to
"current position within current SGE"
After all, it is only the sum of z and x that has any meaning for the
output buffer and thus, the
"number of leading zero bytes in most significant output limb"
part can be moved away from x into z from the beginning, opening up the
opportunity for cleaner code.
Before the outer loop iterating over the SGEs, don't initialize z with
zero, but with the number of leading zero bytes in the most significant
output limb. For the inner loop iterating over a single SGE's bytes,
get rid of the buf_shift offset to x' bounds and let x run from zero to
sg->length - 1.
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 5d02efe..3f114d2 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -477,19 +477,17 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl,
unsigned int nbytes)
j = nlimbs - 1;
a = 0;
- z = 0;
- x = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
- x %= BYTES_PER_MPI_LIMB;
+ z = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
+ z %= BYTES_PER_MPI_LIMB;
for_each_sg(sgl, sg, ents, i) {
const u8 *buffer = sg_virt(sg) + lzeros;
int len = sg->length - lzeros;
- int buf_shift = x;
if (sg_is_last(sg) && (len % BYTES_PER_MPI_LIMB))
len += BYTES_PER_MPI_LIMB - (len % BYTES_PER_MPI_LIMB);
- for (; x < len + buf_shift; x++) {
+ for (x = 0; x < len; x++) {
a <<= 8;
a |= *buffer++;
if (((z + x + 1) % BYTES_PER_MPI_LIMB) == 0) {
@@ -498,7 +496,6 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned
int nbytes)
}
}
z += x;
- x = 0;
lzeros = 0;
}
return val;
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 10/14] lib/mpi: mpi_read_raw_from_sgl(): don't include leading zero SGEs in nbytes
At the very beginning of mpi_read_raw_from_sgl(), the leading zeros of
the input scatterlist are counted:
lzeros = 0;
for_each_sg(sgl, sg, ents, i) {
...
if (/* sg contains nonzero bytes */)
break;
/* sg contains nothing but zeros here */
ents--;
lzeros = 0;
}
Later on, the total number of trailing nonzero bytes is calculated by
subtracting the number of leading zero bytes from the total number of input
bytes:
nbytes -= lzeros;
However, since lzeros gets reset to zero for each completely zero leading
sg in the loop above, it doesn't include those.
Besides wasting resources by allocating a too large output buffer,
this mistake propagates into the calculation of x, the number of
leading zeros within the most significant output limb:
x = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
What's more, the low order bytes of the output, equal in number to the
extra bytes in nbytes, are left uninitialized.
Fix this by adjusting nbytes for each completely zero leading scatterlist
entry.
Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 2de2c7d..add9e81 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -447,16 +447,12 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl,
unsigned int nbytes)
break;
ents--;
+ nbytes -= lzeros;
lzeros = 0;
}
sgl = sg;
-
- if (!ents)
- nbytes = 0;
- else
- nbytes -= lzeros;
-
+ nbytes -= lzeros;
nbits = nbytes * 8;
if (nbits > MAX_EXTERN_MPI_BITS) {
pr_info("MPI: mpi too large (%u bits)\n", nbits);
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 08/14] lib/mpi: mpi_read_buffer(): fix buffer overflow
Currently, mpi_read_buffer() writes full limbs to the output buffer
and moves memory around to purge leading zero limbs afterwards.
However, with
commit 9cbe21d8f89d ("lib/mpi: only require buffers as big as needed for
the integer")
the caller is only required to provide a buffer large enough to hold the
result without the leading zeros.
This might result in a buffer overflow for small MP numbers with leading
zeros.
Fix this by coping the result to its final destination within the output
buffer and not copying the leading zeros at all.
Fixes: 9cbe21d8f89d ("lib/mpi: only require buffers as big as needed for
the integer")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index a999ee1..27582e2 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -201,16 +201,9 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len,
unsigned *nbytes,
#else
#error please implement for this limb size.
#endif
- memcpy(p, &alimb, BYTES_PER_MPI_LIMB);
- p += BYTES_PER_MPI_LIMB;
- if (lzeros > 0) {
- mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
- mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
- + lzeros;
- *limb1 = *limb2;
- p -= lzeros;
- lzeros -= sizeof(alimb);
- }
+ memcpy(p, &alimb + lzeros, BYTES_PER_MPI_LIMB - lzeros);
+ p += BYTES_PER_MPI_LIMB - lzeros;
+ lzeros = 0;
}
return 0;
}
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 09/14] lib/mpi: mpi_read_raw_from_sgl(): replace len argument by nbytes
Currently, the nbytes local variable is calculated from the len argument
as follows:
... mpi_read_raw_from_sgl(..., unsigned int len)
{
unsigned nbytes;
...
if (!ents)
nbytes = 0;
else
nbytes = len - lzeros;
...
}
Given that nbytes is derived from len in a trivial way and that the len
argument is shadowed by a local len variable in several loops, this is just
confusing.
Rename the len argument to nbytes and get rid of the nbytes local variable.
Do the nbytes calculation in place.
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 27582e2..2de2c7d 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -418,15 +418,15 @@ EXPORT_SYMBOL_GPL(mpi_write_to_sgl);
* a new MPI and reads the content of the sgl to the MPI.
*
* @sgl: scatterlist to read from
- * @len: number of bytes to read
+ * @nbytes:number of bytes to read
*
* Return: Pointer to a new MPI or NULL on error
*/
-MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int len)
+MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
{
struct scatterlist *sg;
int x, i, j, z, lzeros, ents;
- unsigned int nbits, nlimbs, nbytes;
+ unsigned int nbits, nlimbs;
mpi_limb_t a;
MPI val = NULL;
@@ -455,7 +455,7 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned
int len)
if (!ents)
nbytes = 0;
else
- nbytes = len - lzeros;
+ nbytes -= lzeros;
nbits = nbytes * 8;
if (nbits > MAX_EXTERN_MPI_BITS) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 12/14] lib/mpi: mpi_read_raw_from_sgl(): fix nbits calculation
The number of bits, nbits, is calculated in mpi_read_raw_from_sgl() as
follows:
nbits = nbytes * 8;
Afterwards, the number of leading zero bits of the first byte get
subtracted:
nbits -= count_leading_zeros(*(u8 *)(sg_virt(sgl) + lzeros));
However, count_leading_zeros() takes an unsigned long and thus,
the u8 gets promoted to an unsigned long.
Thus, the above doesn't subtract the number of leading zeros in the most
significant nonzero input byte from nbits, but the number of leading
zeros of the most significant nonzero input byte promoted to unsigned long,
i.e. BITS_PER_LONG - 8 too many.
Fix this by subtracting
count_leading_zeros(...) - (BITS_PER_LONG - 8)
from nbits only.
Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 4cf8516..5d02efe 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -460,7 +460,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned
int nbytes)
}
if (nbytes > 0)
- nbits -= count_leading_zeros(*(u8 *)(sg_virt(sgl) + lzeros));
+ nbits -= count_leading_zeros(*(u8 *)(sg_virt(sgl) + lzeros)) -
+ (BITS_PER_LONG - 8);
nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
val = mpi_alloc(nlimbs);
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 06/14] lib/mpi: mpi_read_buffer(): optimize skipping of leading zero limbs
Currently, if the number of leading zeros is greater than fits into a
complete limb, mpi_read_buffer() skips them by iterating over them
limb-wise.
Instead of skipping the high order zero limbs within the loop as shown
above, adjust the copying loop's bounds.
Signed-off-by: Nicolai Stange
---
lib/mpi/mpicoder.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 623439e..2fd8d41 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -184,7 +184,9 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len,
unsigned *nbytes,
p = buf;
*nbytes = n - lzeros;
- for (i = a->nlimbs - 1; i >= 0; i--) {
+ for (i = a->nlimbs - 1 - lzeros / BYTES_PER_MPI_LIMB,
+ lzeros %= BYTES_PER_MPI_LIMB;
+ i >= 0; i--) {
alimb = a->d[i];
#if BYTES_PER_MPI_LIMB == 4
*p++ = alimb >> 24;
@@ -205,15 +207,11 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned
buf_len, unsigned *nbytes,
#endif
if (lzeros > 0) {
- if (lzeros >= sizeof(alimb)) {
- p -= sizeof(alimb);
- } else {
- mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
- mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
- + lzeros;
- *limb1 = *limb2;
- p -= lzeros;
- }
+ mpi_limb_t *limb1 = (void *)p - sizeof(alimb);
+ mpi_limb_t *limb2 = (void *)p - sizeof(alimb)
+ + lzeros;
+ *limb1 = *limb2;
+ p -= lzeros;
lzeros -= sizeof(alimb);
}
}
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND v2 00/14] lib/mpi: bug fixes and cleanup
This is a resend of v2 with the crypto people properly CC'd.
The original v1 can be found here:
http://lkml.kernel.org/g/1458237606-4954-1-git-send-email-nicsta...@gmail.com
While v1 (hopefully) fixed some issues in mpi_write_sgl() and
mpi_read_buffer() introduced by
commit 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") and by
commit 9cbe21d8f89d ("lib/mpi: only require buffers as big as needed for
the integer"),
I missed that there are some, including out-of-bounds buffer accesses,
in mpi_read_raw_from_sgl() as well.
Hence v2, which includes the original stuff from v1 plus my new fixes to
mpi_read_raw_from_sgl().
Applicable to linux-next-20160318.
Changes to v1:
- [1-8/14]
former [1-8/8], unchanged.
- [9-14/14]
Added in v2. Fixes to mpi_read_raw_from_sgl().
Nicolai Stange (14):
lib/mpi: mpi_write_sgl(): fix skipping of leading zero limbs
lib/mpi: mpi_write_sgl(): fix style issue with lzero decrement
lib/mpi: mpi_write_sgl(): purge redundant pointer arithmetic
lib/mpi: mpi_write_sgl(): fix out-of-bounds stack access
lib/mpi: mpi_write_sgl(): replace open coded endian conversion
lib/mpi: mpi_read_buffer(): optimize skipping of leading zero limbs
lib/mpi: mpi_read_buffer(): replace open coded endian conversion
lib/mpi: mpi_read_buffer(): fix buffer overflow
lib/mpi: mpi_read_raw_from_sgl(): replace len argument by nbytes
lib/mpi: mpi_read_raw_from_sgl(): don't include leading zero SGEs in
nbytes
lib/mpi: mpi_read_raw_from_sgl(): purge redundant clearing of nbits
lib/mpi: mpi_read_raw_from_sgl(): fix nbits calculation
lib/mpi: mpi_read_raw_from_sgl(): sanitize meaning of indices
lib/mpi: mpi_read_raw_from_sgl(): fix out-of-bounds buffer access
lib/mpi/mpicoder.c | 122 +++--
1 file changed, 43 insertions(+), 79 deletions(-)
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 10/10] crypto: caam - add support for RSA algorithm
Hi Stephan,
> -Original Message-
> From: Stephan Mueller [mailto:smuel...@chronox.de]
> Sent: Saturday, March 19, 2016 6:59 PM
> To: Tudor-Dan Ambarus
> Cc: herb...@gondor.apana.org.au; tadeusz.st...@intel.com; linux-
> cry...@vger.kernel.org; Horia Ioan Geanta Neag
> Subject: Re: [PATCH 10/10] crypto: caam - add support for RSA algorithm
>
> > +void rsa_free_key(struct rsa_raw_key *key)
> > +{
> > + kzfree(key->d);
> > + key->d = NULL;
> > +
> > + kfree(key->e);
> > + key->e = NULL;
> > +
> > + kfree(key->n);
> > + key->n = NULL;
> > +
> > + key->n_sz = 0;
> > + key->e_sz = 0;
> > +}
>
> As you implement raw key support for use in other drivers, shouldn't that
> function go into some helper file like free_mpis().
> > +
[ta] I should also add an rsa_free_coherent_key(struct device *dev, struct
rsa_raw_key *key), for those implementations that use the DMA-coherent API.
> > +static int caam_rsa_dec(struct akcipher_request *req)
> > +{
> > + struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> > + struct rsa_raw_ctx *ctx = akcipher_tfm_ctx(tfm);
> > + struct rsa_raw_key *key = &ctx->key;
> > + struct device *jrdev = ctx->dev;
> > + struct rsa_edesc *edesc = NULL;
> > + size_t desclen = sizeof(struct rsa_priv_f1_desc);
> > + int ret;
> > +
> > + if (unlikely(!key->n || !key->d))
> > + return -EINVAL;
> > +
> > + if (req->dst_len < key->n_sz) {
> > + req->dst_len = key->n_sz;
> > + return -EOVERFLOW;
> > + }
> > +
> > + /* Allocate extended descriptor. */
> > + edesc = rsa_edesc_alloc(req, desclen);
> > + if (IS_ERR(edesc))
> > + return PTR_ERR(edesc);
> > +
> > + /* Initialize Job Descriptor. */
> > + ret = init_rsa_priv_f1_desc(req, edesc);
> > + if (ret)
> > + return ret;
>
> Same here, kfree?
[ta] Sure, thanks.
> > +
> > + ret = caam_jr_enqueue(jrdev, edesc->hw_desc, rsa_priv_f1_done, req);
> > + if (!ret) {
> > + ret = -EINPROGRESS;
>
> dto
>
[ta] resources are freed on rsa_priv_f1_done callback.
> > +static struct akcipher_alg rsa = {
>
> can you please name that something else, like caam_rsa? It is a real hassle
> when searching some symbol and I get such a generic name.
[ta] ok.
Thank you for the review!
ta
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 07/10] crypto: qat - remove duplicate ASN.1 parser
> -Original Message- > From: Tadeusz Struk [mailto:tadeusz.st...@intel.com] > Sent: Sunday, March 20, 2016 4:56 PM > To: Tudor-Dan Ambarus; herb...@gondor.apana.org.au > Cc: linux-crypto@vger.kernel.org; smuel...@chronox.de; Horia Ioan Geanta > Neag > Subject: Re: [PATCH 07/10] crypto: qat - remove duplicate ASN.1 parser > > On 03/18/2016 11:32 AM, Tudor Ambarus wrote: > > Use the RSA's software implementation parser with > > raw integer actions. > > > > Compile-tested only. > > > > Signed-off-by: Tudor Ambarus > > --- > > drivers/crypto/qat/Kconfig| 3 +- > > drivers/crypto/qat/qat_common/Makefile| 10 +- > > drivers/crypto/qat/qat_common/qat_asym_algs.c | 265 +++- > -- > > drivers/crypto/qat/qat_common/qat_rsaprivkey.asn1 | 11 - > > drivers/crypto/qat/qat_common/qat_rsapubkey.asn1 | 4 - > > 5 files changed, 83 insertions(+), 210 deletions(-) > > delete mode 100644 drivers/crypto/qat/qat_common/qat_rsaprivkey.asn1 > > delete mode 100644 drivers/crypto/qat/qat_common/qat_rsapubkey.asn1 > > Thanks for converting qat, but I'm working on adding support for the > Chinese > remainder algorithm, which gives us big performance improvement, so please > do not change the qat driver just yet. > Please just add a generic ANS.1 parser that would allow to obtain all > components of the private key and I'll use it to add the handlers > and remove the ASN.1 parsing from qat. I will then test it to make sure > that it works fine before it is committed. [ta] Ok. > Thanks, > -- > TS -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/10] crypto: rsa - generalize ASN.1 sequences
Hi Tadeusz,
> -Original Message-
> From: Tadeusz Struk [mailto:tadeusz.st...@intel.com]
> Sent: Sunday, March 20, 2016 4:54 PM
> To: Tudor-Dan Ambarus; herb...@gondor.apana.org.au
> Cc: linux-crypto@vger.kernel.org; smuel...@chronox.de; Horia Ioan Geanta
> Neag
> Subject: Re: [PATCH 01/10] crypto: rsa - generalize ASN.1 sequences
> static int rsa_set_priv_key(struct crypto_akcipher *tfm, const void *key,
> unsigned int keylen)
> {
> struct rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
> struct rsa_mpi_key *pkey = &ctx->key;
> int ret;
>
> ctx->action = &impl_action;
[ta] we can do this once, when initializing the tfm object.
I agree with all your suggestions, I will implement them.
Thank you for the review,
ta
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] crypto: marvell/cesa - remove unneeded condition
On Mon, 21 Mar 2016 12:03:43 +0300 Dan Carpenter wrote: > creq->cache[] is an array inside the struct, it's not a pointer and it > can't be NULL. > > Signed-off-by: Dan Carpenter Acked-by: Boris Brezillon > > diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c > index 7ca2e0f..7a5058d 100644 > --- a/drivers/crypto/marvell/hash.c > +++ b/drivers/crypto/marvell/hash.c > @@ -768,8 +768,7 @@ static int mv_cesa_ahash_export(struct ahash_request > *req, void *hash, > *len = creq->len; > memcpy(hash, creq->state, digsize); > memset(cache, 0, blocksize); > - if (creq->cache) > - memcpy(cache, creq->cache, creq->cache_ptr); > + memcpy(cache, creq->cache, creq->cache_ptr); > > return 0; > } -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] crypto: marvell/cesa - remove unneeded condition
creq->cache[] is an array inside the struct, it's not a pointer and it can't be NULL. Signed-off-by: Dan Carpenter diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c index 7ca2e0f..7a5058d 100644 --- a/drivers/crypto/marvell/hash.c +++ b/drivers/crypto/marvell/hash.c @@ -768,8 +768,7 @@ static int mv_cesa_ahash_export(struct ahash_request *req, void *hash, *len = creq->len; memcpy(hash, creq->state, digsize); memset(cache, 0, blocksize); - if (creq->cache) - memcpy(cache, creq->cache, creq->cache_ptr); + memcpy(cache, creq->cache, creq->cache_ptr); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
