Re: [PATCH RESEND v2 00/14] lib/mpi: bug fixes and cleanup

[Tadeusz Struk]

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-nicstange@xxxxxxxxx
> 
> 
> 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 <tadeusz.struk@xxxxxxxxx>
---
 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 <tadeusz.struk@xxxxxxxxx>
for the whole series.
Thanks,
-- 
TS