| Message ID | 3348160.hjCpAHFZrS@positron.chronox.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Aug 10, 2017 at 08:06:18AM +0200, Stephan Müller wrote: > Hi Herbert, > > I found that issue while playing around with edge conditions in my > algif_akcipher implementation. This issue only manifests in a > segmentation violation on 32 bit machines and with an SGL where each > SG points to one byte. SGLs with larger buffers seem to be not > affected by this issue. > > Yet this access-after-unmap should be a candidate for stable, IMHO. Good catch. Thanks! Fixes: 4816c9406430 ("lib/mpi: Fix SG miter leak") Cc: <stable@vger.kernel.org>
On Thu, Aug 10, 2017 at 08:06:18AM +0200, Stephan Müller wrote: > Hi Herbert, > > I found that issue while playing around with edge conditions in my > algif_akcipher implementation. This issue only manifests in a > segmentation violation on 32 bit machines and with an SGL where each > SG points to one byte. SGLs with larger buffers seem to be not > affected by this issue. > > Yet this access-after-unmap should be a candidate for stable, IMHO. > > ---8<--- > > Using sg_miter_start and sg_miter_next, the buffer of an SG is kmap'ed > to *buff. The current code calls sg_miter_stop (and thus kunmap) on the > SG entry before the last access of *buff. > > The patch moves the sg_miter_stop call after the last access to *buff to > ensure that the memory pointed to by *buff is still mapped. > > Signed-off-by: Stephan Mueller <smueller@chronox.de> Patch applied. Thanks.
Am Dienstag, 22. August 2017, 08:33:15 CEST schrieb Herbert Xu: Hi Herbert, > On Thu, Aug 10, 2017 at 08:06:18AM +0200, Stephan Müller wrote: > > Hi Herbert, > > > > I found that issue while playing around with edge conditions in my > > algif_akcipher implementation. This issue only manifests in a > > segmentation violation on 32 bit machines and with an SGL where each > > SG points to one byte. SGLs with larger buffers seem to be not > > affected by this issue. > > > > Yet this access-after-unmap should be a candidate for stable, IMHO. > > Good catch. Thanks! > > Fixes: 4816c9406430 ("lib/mpi: Fix SG miter leak") > Cc: <stable@vger.kernel.org> Just to confirm: stable is not CCed in this email -- did you send it separately to stable? (dto for "[PATCH v4] crypto: only call put_page on referenced and used pages") Thanks a lot Stephan
On Wed, Aug 30, 2017 at 06:33:41PM +0200, Stephan Mueller wrote: > Am Dienstag, 22. August 2017, 08:33:15 CEST schrieb Herbert Xu: > > Hi Herbert, > > > On Thu, Aug 10, 2017 at 08:06:18AM +0200, Stephan Müller wrote: > > > Hi Herbert, > > > > > > I found that issue while playing around with edge conditions in my > > > algif_akcipher implementation. This issue only manifests in a > > > segmentation violation on 32 bit machines and with an SGL where each > > > SG points to one byte. SGLs with larger buffers seem to be not > > > affected by this issue. > > > > > > Yet this access-after-unmap should be a candidate for stable, IMHO. > > > > Good catch. Thanks! > > > > Fixes: 4816c9406430 ("lib/mpi: Fix SG miter leak") > > Cc: <stable@vger.kernel.org> > > Just to confirm: stable is not CCed in this email -- did you send it > separately to stable? (dto for "[PATCH v4] crypto: only call put_page on > referenced and used pages") The cc is meant to go into the commit and when the commit hits mainline it'll automatically get picked up by stable. Cheers,
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c index 5a0f75a3bf01..eead4b339466 100644 --- a/lib/mpi/mpicoder.c +++ b/lib/mpi/mpicoder.c @@ -364,11 +364,11 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes) } miter.consumed = lzeros; - sg_miter_stop(&miter); nbytes -= lzeros; nbits = nbytes * 8; if (nbits > MAX_EXTERN_MPI_BITS) { + sg_miter_stop(&miter); pr_info("MPI: mpi too large (%u bits)\n", nbits); return NULL; } @@ -376,6 +376,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes) if (nbytes > 0) nbits -= count_leading_zeros(*buff) - (BITS_PER_LONG - 8); + sg_miter_stop(&miter); + nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB); val = mpi_alloc(nlimbs); if (!val)
Hi Herbert, I found that issue while playing around with edge conditions in my algif_akcipher implementation. This issue only manifests in a segmentation violation on 32 bit machines and with an SGL where each SG points to one byte. SGLs with larger buffers seem to be not affected by this issue. Yet this access-after-unmap should be a candidate for stable, IMHO. ---8<--- Using sg_miter_start and sg_miter_next, the buffer of an SG is kmap'ed to *buff. The current code calls sg_miter_stop (and thus kunmap) on the SG entry before the last access of *buff. The patch moves the sg_miter_stop call after the last access to *buff to ensure that the memory pointed to by *buff is still mapped. Signed-off-by: Stephan Mueller <smueller@chronox.de> --- lib/mpi/mpicoder.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)