[PATCH 0/5] refactor mpi_read_from_buffer()

[PATCH 0/5] refactor mpi_read_from_buffer()

[Nicolai Stange]

mpi_read_from_buffer() and mpi_read_raw_data() do almost the same and share a
fair amount of common code.

This patchset attempts to rewrite mpi_read_from_buffer() in order to implement
it in terms of mpi_read_raw_data().

The patches 1 and 3, i.e.
  "lib/mpi: mpi_read_from_buffer(): return error code"
and
  "lib/mpi: mpi_read_from_buffer(): return -EINVAL upon too short buffer"
do the groundwork in that they move any error detection unique to
mpi_read_from_buffer() out of the data handling loop.

The patches 2 and 4, that is
  "lib/digsig: digsig_verify_rsa(): return -EINVAL if modulo length is zero"
and
  "lib/mpi: mpi_read_from_buffer(): sanitize short buffer printk"
are not strictly necessary for the refactoring: they cleanup some minor oddities
related to error handling I came across.

Finally, the last patch in this series,
  "lib/mpi: refactor mpi_read_from_buffer() in terms of mpi_read_raw_data()"
actually does what this series is all about.


Applicable to linux-next-20160325.

A note on testing: allmodconfig on x86_64 builds fine. 
However, the only caller of mpi_read_from_buffer() is digsig_verify_rsa()
and this one is solely used by the IMA/EVM infrastructure.
In my current setup, I don't have any IMA/EVM stuff in place and thus,
I can't do any runtime tests without putting *much* effort into it.
I would really appreciate if someone with a working IMA/EVM setup could do some
brief testing...

Nicolai Stange (5):
  lib/mpi: mpi_read_from_buffer(): return error code
  lib/digsig: digsig_verify_rsa(): return -EINVAL if modulo length is
    zero
  lib/mpi: mpi_read_from_buffer(): return -EINVAL upon too short buffer
  lib/mpi: mpi_read_from_buffer(): sanitize short buffer printk
  lib/mpi: refactor mpi_read_from_buffer() in terms of
    mpi_read_raw_data()

 lib/digsig.c       | 16 +++++++++++-----
 lib/mpi/mpicoder.c | 46 +++++++++++++---------------------------------
 2 files changed, 24 insertions(+), 38 deletions(-)

-- 
2.8.2

[PATCH 1/5] lib/mpi: mpi_read_from_buffer(): return error code

[Nicolai Stange]

mpi_read_from_buffer() reads a MPI from a buffer into a newly allocated
MPI instance. It expects the buffer's leading two bytes to contain the
number of bits, followed by the actual payload.

On failure, it returns NULL and updates the in/out argument ret_nread
somewhat inconsistently:
- If the given buffer is too short to contain the leading two bytes
  encoding the number of bits or their value is unsupported, then
  ret_nread will be cleared.
- If the allocation of the resulting MPI instance fails, ret_nread is left
  as is.

The only user of mpi_read_from_buffer(), digsig_verify_rsa(), simply checks
for a return value of NULL and returns -ENOMEM if that happens.

While this is all of cosmetic nature only, there is another error condition
which currently isn't detectable by the caller of mpi_read_from_buffer():
if the given buffer is too small to hold the number of bits as encoded in
its first two bytes, the return value will be non-NULL and *ret_nread > 0.

In preparation of communicating this condition to the caller, let
mpi_read_from_buffer() return error values by means of the ERR_PTR()
mechanism.

Make the sole caller of mpi_read_from_buffer(), digsig_verify_rsa(),
check the return value for IS_ERR() rather than == NULL. If IS_ERR() is
true, return the associated error value rather than the fixed -ENOMEM.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 lib/digsig.c       | 12 ++++++++----
 lib/mpi/mpicoder.c |  6 +++---
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/digsig.c b/lib/digsig.c
index 07be6c1..a121cbc 100644
--- a/lib/digsig.c
+++ b/lib/digsig.c
@@ -104,16 +104,18 @@ static int digsig_verify_rsa(struct key *key,
 	datap = pkh->mpi;
 	endp = ukp->data + ukp->datalen;
 
-	err = -ENOMEM;
-
 	for (i = 0; i < pkh->nmpi; i++) {
 		unsigned int remaining = endp - datap;
 		pkey[i] = mpi_read_from_buffer(datap, &remaining);
-		if (!pkey[i])
+		if (IS_ERR(pkey[i])) {
+			err = PTR_ERR(pkey[i]);
 			goto err;
+		}
 		datap += remaining;
 	}
 
+	err = -ENOMEM;
+
 	mblen = mpi_get_nbits(pkey[0]);
 	mlen = DIV_ROUND_UP(mblen, 8);
 
@@ -126,8 +128,10 @@ static int digsig_verify_rsa(struct key *key,
 
 	nret = siglen;
 	in = mpi_read_from_buffer(sig, &nret);
-	if (!in)
+	if (IS_ERR(in)) {
+		err = PTR_ERR(in);
 		goto err;
+	}
 
 	res = mpi_alloc(mpi_get_nlimbs(in) * 2);
 	if (!res)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 747606f..275c71e 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -88,12 +88,12 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 	MPI val = NULL;
 
 	if (*ret_nread < 2)
-		goto leave;
+		return ERR_PTR(-EINVAL);
 	nbits = buffer[0] << 8 | buffer[1];
 
 	if (nbits > MAX_EXTERN_MPI_BITS) {
 		pr_info("MPI: mpi too large (%u bits)\n", nbits);
-		goto leave;
+		return ERR_PTR(-EINVAL);
 	}
 	buffer += 2;
 	nread = 2;
@@ -102,7 +102,7 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 	nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
 	val = mpi_alloc(nlimbs);
 	if (!val)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	i = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
 	i %= BYTES_PER_MPI_LIMB;
 	val->nbits = nbits;
-- 
2.8.2

[PATCH 2/5] lib/digsig: digsig_verify_rsa(): return -EINVAL if modulo length is zero

[Nicolai Stange]

Currently, if digsig_verify_rsa() detects that the modulo's length is zero,
i.e. mlen == 0, it returns -ENOMEM which doesn't really fit here.

Make digsig_verify_rsa() return -EINVAL upon mlen == 0.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 lib/digsig.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/digsig.c b/lib/digsig.c
index a121cbc..55b8b2f 100644
--- a/lib/digsig.c
+++ b/lib/digsig.c
@@ -114,13 +114,15 @@ static int digsig_verify_rsa(struct key *key,
 		datap += remaining;
 	}
 
-	err = -ENOMEM;
-
 	mblen = mpi_get_nbits(pkey[0]);
 	mlen = DIV_ROUND_UP(mblen, 8);
 
-	if (mlen == 0)
+	if (mlen == 0) {
+		err = -EINVAL;
 		goto err;
+	}
+
+	err = -ENOMEM;
 
 	out1 = kzalloc(mlen, GFP_KERNEL);
 	if (!out1)
-- 
2.8.2

[PATCH 3/5] lib/mpi: mpi_read_from_buffer(): return -EINVAL upon too short buffer

[Nicolai Stange]

Currently, if the input buffer is shorter than the expected length as
indicated by its first two bytes, an MPI instance of this expected length
will be allocated and filled with as much data as is available. The rest
will remain uninitialized.

Instead of leaving this condition undetected, an error code should be
reported to the caller.

Since this situation indicates that the input buffer's first two bytes,
encoding the number of expected bits, are garbled, -EINVAL is appropriate
here.

If the input buffer is shorter than indicated by its first two bytes,
make mpi_read_from_buffer() return -EINVAL.
Get rid of the 'nread' variable: with the new semantics, the total number
of bytes read from the input buffer is known in advance.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 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 275c71e..869c66c 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -83,7 +83,7 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 {
 	const uint8_t *buffer = xbuffer;
 	int i, j;
-	unsigned nbits, nbytes, nlimbs, nread = 0;
+	unsigned nbits, nbytes, nlimbs;
 	mpi_limb_t a;
 	MPI val = NULL;
 
@@ -96,9 +96,14 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 		return ERR_PTR(-EINVAL);
 	}
 	buffer += 2;
-	nread = 2;
 
 	nbytes = DIV_ROUND_UP(nbits, 8);
+	if (nbytes + 2 > *ret_nread) {
+		printk("MPI: mpi larger than buffer nread=%d ret_nread=%d\n",
+				*ret_nread + 1, *ret_nread);
+		return ERR_PTR(-EINVAL);
+	}
+
 	nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
 	val = mpi_alloc(nlimbs);
 	if (!val)
@@ -111,12 +116,6 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 	for (; j > 0; j--) {
 		a = 0;
 		for (; i < BYTES_PER_MPI_LIMB; i++) {
-			if (++nread > *ret_nread) {
-				printk
-				    ("MPI: mpi larger than buffer nread=%d ret_nread=%d\n",
-				     nread, *ret_nread);
-				goto leave;
-			}
 			a <<= 8;
 			a |= *buffer++;
 		}
@@ -124,8 +123,7 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 		val->d[j - 1] = a;
 	}
 
-leave:
-	*ret_nread = nread;
+	*ret_nread = nbytes + 2;
 	return val;
 }
 EXPORT_SYMBOL_GPL(mpi_read_from_buffer);
-- 
2.8.2

[PATCH 4/5] lib/mpi: mpi_read_from_buffer(): sanitize short buffer printk

[Nicolai Stange]

The first two bytes of the input buffer encode its expected length and
mpi_read_from_buffer() prints a console message if the given buffer is too
short.

However, there are some oddities with how this message is printed:
- It is printed at the default loglevel. This is different from the
  one used in the case that the first two bytes' value is unsupportedly
  large, i.e. KERN_INFO.
- The format specifier '%d' is used for unsigned ints.
- It prints the values of nread and *ret_nread. This is redundant since
  the former is always the latter + 1.

Clean this up as follows:
- Use pr_info() rather than printk() with no loglevel.
- Use the format specifiers '%u' in place if '%d'.
- Do not print the redundant 'nread' but the more helpful 'nbytes' value.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 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 869c66c..2f4d039 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -99,8 +99,8 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 
 	nbytes = DIV_ROUND_UP(nbits, 8);
 	if (nbytes + 2 > *ret_nread) {
-		printk("MPI: mpi larger than buffer nread=%d ret_nread=%d\n",
-				*ret_nread + 1, *ret_nread);
+		pr_info("MPI: mpi larger than buffer nbytes=%u ret_nread=%u\n",
+				nbytes, *ret_nread);
 		return ERR_PTR(-EINVAL);
 	}
 
-- 
2.8.2

[PATCH 5/5] lib/mpi: refactor mpi_read_from_buffer() in terms of mpi_read_raw_data()

[Nicolai Stange]

mpi_read_from_buffer() and mpi_read_raw_data() do basically the same thing
except that the former extracts the number of payload bits from the first
two bytes of the input buffer.

Besides that, the data copying logic is exactly the same.

Replace the open coded buffer to MPI instance conversion by a call to
mpi_read_raw_data().

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 lib/mpi/mpicoder.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 2f4d039..e8a5742 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -82,10 +82,8 @@ EXPORT_SYMBOL_GPL(mpi_read_raw_data);
 MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 {
 	const uint8_t *buffer = xbuffer;
-	int i, j;
-	unsigned nbits, nbytes, nlimbs;
-	mpi_limb_t a;
-	MPI val = NULL;
+	unsigned int nbits, nbytes;
+	MPI val;
 
 	if (*ret_nread < 2)
 		return ERR_PTR(-EINVAL);
@@ -95,7 +93,6 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 		pr_info("MPI: mpi too large (%u bits)\n", nbits);
 		return ERR_PTR(-EINVAL);
 	}
-	buffer += 2;
 
 	nbytes = DIV_ROUND_UP(nbits, 8);
 	if (nbytes + 2 > *ret_nread) {
@@ -104,24 +101,9 @@ MPI mpi_read_from_buffer(const void *xbuffer, unsigned *ret_nread)
 		return ERR_PTR(-EINVAL);
 	}
 
-	nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
-	val = mpi_alloc(nlimbs);
+	val = mpi_read_raw_data(buffer + 2, nbytes);
 	if (!val)
 		return ERR_PTR(-ENOMEM);
-	i = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
-	i %= BYTES_PER_MPI_LIMB;
-	val->nbits = nbits;
-	j = val->nlimbs = nlimbs;
-	val->sign = 0;
-	for (; j > 0; j--) {
-		a = 0;
-		for (; i < BYTES_PER_MPI_LIMB; i++) {
-			a <<= 8;
-			a |= *buffer++;
-		}
-		i = 0;
-		val->d[j - 1] = a;
-	}
 
 	*ret_nread = nbytes + 2;
 	return val;
-- 
2.8.2

Re: [PATCH 0/5] refactor mpi_read_from_buffer()

[Herbert Xu]

On Thu, May 26, 2016 at 11:19:50PM +0200, Nicolai Stange wrote:
> mpi_read_from_buffer() and mpi_read_raw_data() do almost the same and share a
> fair amount of common code.
> 
> This patchset attempts to rewrite mpi_read_from_buffer() in order to implement
> it in terms of mpi_read_raw_data().
> 
> The patches 1 and 3, i.e.
>   "lib/mpi: mpi_read_from_buffer(): return error code"
> and
>   "lib/mpi: mpi_read_from_buffer(): return -EINVAL upon too short buffer"
> do the groundwork in that they move any error detection unique to
> mpi_read_from_buffer() out of the data handling loop.
> 
> The patches 2 and 4, that is
>   "lib/digsig: digsig_verify_rsa(): return -EINVAL if modulo length is zero"
> and
>   "lib/mpi: mpi_read_from_buffer(): sanitize short buffer printk"
> are not strictly necessary for the refactoring: they cleanup some minor oddities
> related to error handling I came across.
> 
> Finally, the last patch in this series,
>   "lib/mpi: refactor mpi_read_from_buffer() in terms of mpi_read_raw_data()"
> actually does what this series is all about.
> 
> 
> Applicable to linux-next-20160325.

All applied.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/5] refactor mpi_read_from_buffer()

[Nicolai Stange]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Thu, May 26, 2016 at 11:19:50PM +0200, Nicolai Stange wrote:
>> mpi_read_from_buffer() and mpi_read_raw_data() do almost the same and share a
>> fair amount of common code.
>> 
>> This patchset attempts to rewrite mpi_read_from_buffer() in order to implement
>> it in terms of mpi_read_raw_data().
>> 
>> The patches 1 and 3, i.e.
>>   "lib/mpi: mpi_read_from_buffer(): return error code"
>> and
>>   "lib/mpi: mpi_read_from_buffer(): return -EINVAL upon too short buffer"
>> do the groundwork in that they move any error detection unique to
>> mpi_read_from_buffer() out of the data handling loop.
>> 
>> The patches 2 and 4, that is
>>   "lib/digsig: digsig_verify_rsa(): return -EINVAL if modulo length is zero"
>> and
>>   "lib/mpi: mpi_read_from_buffer(): sanitize short buffer printk"
>> are not strictly necessary for the refactoring: they cleanup some minor oddities
>> related to error handling I came across.
>> 
>> Finally, the last patch in this series,
>>   "lib/mpi: refactor mpi_read_from_buffer() in terms of mpi_read_raw_data()"
>> actually does what this series is all about.
>> 
>> 
>> Applicable to linux-next-20160325.
>
> All applied.

Thanks! (As well as for applying the separately sent patches, of course)