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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
