Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526332 > unrolled thread

Re: vmalloced stacks and scatterwalk_map_and_copy()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: vmalloced stacks and scatterwalk_map_and_copy() Andy Lutomirski <luto@amacapital.net> - 2016-11-21 03:30 +0100
    Re: vmalloced stacks and scatterwalk_map_and_copy() Herbert Xu <herbert@gondor.apana.org.au> - 2016-11-21 09:30 +0100
      Re: vmalloced stacks and scatterwalk_map_and_copy() Eric Biggers <ebiggers@google.com> - 2016-11-21 19:10 +0100

#1526332 — Re: vmalloced stacks and scatterwalk_map_and_copy()

[Adding Thorsten to help keep this from getting lost]

On Thu, Nov 3, 2016 at 1:30 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Nov 3, 2016 at 11:16 AM, Eric Biggers <ebiggers@google.com> wrote:
>> Hello,
>>
>> I hit the BUG_ON() in arch/x86/mm/physaddr.c:26 while testing some crypto code
>> in an x86_64 kernel with CONFIG_DEBUG_VIRTUAL=y and CONFIG_VMAP_STACK=y:
>>
>>         /* carry flag will be set if starting x was >= PAGE_OFFSET */
>>         VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
>>
>> The problem is the following code in scatterwalk_map_and_copy() in
>> crypto/scatterwalk.c, which tries to determine if the buffer passed in aliases
>> the physical memory of the first segment of the scatterlist:
>>
>>         if (sg_page(sg) == virt_to_page(buf) &&
>>             sg->offset == offset_in_page(buf))
>>                 return;
>
> ...
>
>>
>> Currently I think the best solution would be to require that callers to
>> scatterwalk_map_and_copy() do not alias their source and destination.  Then the
>> alias check could be removed.  This check has only been there since v4.2 (commit
>> 74412fd5d71b6), so I'd hope not many callers rely on the behavior.  I'm not sure
>> exactly which ones do, though.
>>
>> Thoughts on this?
>
> The relevant commit is:
>
> commit 74412fd5d71b6eda0beb302aa467da000f0d530c
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu May 21 15:11:12 2015 +0800
>
>     crypto: scatterwalk - Check for same address in map_and_copy
>
>     This patch adds a check for in scatterwalk_map_and_copy to avoid
>     copying from the same address to the same address.  This is going
>     to be used for IV copying in AEAD IV generators.
>
>     There is no provision for partial overlaps.
>
>     This patch also uses the new scatterwalk_ffwd instead of doing
>     it by hand in scatterwalk_map_and_copy.
>
>     Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> 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?
>
> Also, Herbert, it seems like the considerable majority of the crypto
> code is acting on kernel virtual memory addresses and does software
> processing.  Would it perhaps make sense to add a kvec-based or
> iov_iter-based interface to the crypto code?  I bet it would be quite
> a bit faster and it would make crypto on stack buffers work directly.


Ping, everyone!

It's getting quite close to 4.9 release time.  Is there an actual bug
here?  Because, if so, we need to fix it.  My preference is to just
delete the weird aliasing check, but it would be really nice to know
if that check is needed for some reason.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [next] | [standalone]

#1526450

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>

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);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1526960

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web