From: Tadeusz Struk Subject: Re: [PATCH RESEND v2 00/14] lib/mpi: bug fixes and cleanup Date: Mon, 21 Mar 2016 21:54:36 -0700 Message-ID: <56F0D00C.7000005@intel.com> References: <1458566775-5239-1-git-send-email-nicstange@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: Michal Marek , Andrzej Zaborowski , Stephan Mueller , Arnd Bergmann , linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org To: Nicolai Stange , Herbert Xu , "David S. Miller" Return-path: Received: from mga01.intel.com ([192.55.52.88]:46002 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750824AbcCVE71 (ORCPT ); Tue, 22 Mar 2016 00:59:27 -0400 In-Reply-To: <1458566775-5239-1-git-send-email-nicstange@gmail.com> Sender: linux-crypto-owner@vger.kernel.org List-ID: 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@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