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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56F0D00C.7000005@intel.com>
Date:	Mon, 21 Mar 2016 21:54:36 -0700
From:	Tadeusz Struk <tadeusz.struk@...el.com>
To:	Nicolai Stange <nicstange@...il.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>
Cc:	Michal Marek <mmarek@...e.com>,
	Andrzej Zaborowski <andrew.zaborowski@...el.com>,
	Stephan Mueller <smueller@...onox.de>,
	Arnd Bergmann <arnd@...db.de>, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: 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-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 <tadeusz.struk@...el.com>
---
 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@...el.com>
for the whole series.
Thanks,
-- 
TS