diff mbox

vmalloced stacks and scatterwalk_map_and_copy()

Message ID 20161121082619.GA6462@gondor.apana.org.au (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show

Commit Message

Herbert Xu Nov. 21, 2016, 8:26 a.m. UTC 
On Sun, Nov 20, 2016 at 06:19:48PM -0800, Andy Lutomirski wrote:
>
> > Herbert, can you clarify this?  The check seems rather bizarre --
> > you're doing an incomplete check for aliasing and skipping the whole
> > copy if the beginning aliases.  In any event the stack *can't*
> > reasonably alias the scatterlist because a scatterlist can't safely
> > point to the stack.  Is there any code that actually relies on the
> > aliasing-detecting behavior?

Well at the time the IPsec stack would pass an IV that pointed
into the actual request, which is what prompted that patch.  The
IPsec code has since been changed to provide a separate IV so this
check is no longer necessary.

I will remove it with this patch.

---8<---
crypto: scatterwalk - Remove unnecessary aliasing check in map_and_copy

The aliasing check in map_and_copy is no longer necessary because
the IPsec ESP code no longer provides an IV that points into the
actual request data.  As this check is now triggering BUG checks
due to the vmalloced stack code, I'm removing it.

Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Eric Biggers Nov. 21, 2016, 6:08 p.m. UTC | #1 
On Mon, Nov 21, 2016 at 04:26:19PM +0800, Herbert Xu wrote:
> crypto: scatterwalk - Remove unnecessary aliasing check in map_and_copy
> 
> The aliasing check in map_and_copy is no longer necessary because
> the IPsec ESP code no longer provides an IV that points into the
> actual request data.  As this check is now triggering BUG checks
> due to the vmalloced stack code, I'm removing it.
> 
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
> index 52ce17a..c16c94f8 100644
> --- a/crypto/scatterwalk.c
> +++ b/crypto/scatterwalk.c
> @@ -68,10 +68,6 @@ void scatterwalk_map_and_copy(void *buf, struct scatterlist *sg,
>  
>  	sg = scatterwalk_ffwd(tmp, sg, start);
>  
> -	if (sg_page(sg) == virt_to_page(buf) &&
> -	    sg->offset == offset_in_page(buf))
> -		return;
> -
>  	scatterwalk_start(&walk, sg);
>  	scatterwalk_copychunks(buf, &walk, nbytes, out);
>  	scatterwalk_done(&walk, out, 0);

This looks fine to me if you're confident that the aliasing check is indeed no
longer necessary.

Another idea I had was to replace memcpy() with memmove().  But I don't want to
be in a situation where we're stuck with memmove() forever because of users who
probably don't even exist.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index 52ce17a..c16c94f8 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -68,10 +68,6 @@  void scatterwalk_map_and_copy(void *buf, struct scatterlist *sg,
 
 	sg = scatterwalk_ffwd(tmp, sg, start);
 
-	if (sg_page(sg) == virt_to_page(buf) &&
-	    sg->offset == offset_in_page(buf))
-		return;
-
 	scatterwalk_start(&walk, sg);
 	scatterwalk_copychunks(buf, &walk, nbytes, out);
 	scatterwalk_done(&walk, out, 0);