* [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
@ 2020-07-16 15:29 Colin King
2020-07-16 15:56 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Colin King @ 2020-07-16 15:29 UTC (permalink / raw)
To: Herbert Xu, David S . Miller, Ard Biesheuvel, linux-crypto
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There is a memcpy that performs a potential overlapped memory copy
from source b to destination b + 1. Fix this by using the safer
memmove instead.
Addresses-Coverity: ("Overlapping buffer in memory copy")
Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
crypto/xts.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/xts.c b/crypto/xts.c
index 3565f3b863a6..fa3e6e7b7043 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -169,7 +169,7 @@ static int cts_final(struct skcipher_request *req,
offset - XTS_BLOCK_SIZE);
scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
- memcpy(b + 1, b, tail);
+ memmove(b + 1, b, tail);
scatterwalk_map_and_copy(b, req->src, offset, tail, 0);
le128_xor(b, &rctx->t, b);
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
2020-07-16 15:29 [PATCH] crypto: xts: use memmove to avoid overlapped memory copy Colin King
@ 2020-07-16 15:56 ` Ard Biesheuvel
2020-07-16 16:05 ` Colin Ian King
2020-07-17 5:21 ` Herbert Xu
0 siblings, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-07-16 15:56 UTC (permalink / raw)
To: Colin King
Cc: Herbert Xu, David S . Miller, Linux Crypto Mailing List,
kernel-janitors, Linux Kernel Mailing List
On Thu, 16 Jul 2020 at 18:29, Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a memcpy that performs a potential overlapped memory copy
> from source b to destination b + 1. Fix this by using the safer
> memmove instead.
>
> Addresses-Coverity: ("Overlapping buffer in memory copy")
> Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> crypto/xts.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/xts.c b/crypto/xts.c
> index 3565f3b863a6..fa3e6e7b7043 100644
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -169,7 +169,7 @@ static int cts_final(struct skcipher_request *req,
> offset - XTS_BLOCK_SIZE);
>
> scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
> - memcpy(b + 1, b, tail);
> + memmove(b + 1, b, tail);
This is a false positive: tail is guaranteed to be smaller than
sizeof(*b), so memmove() is unnecessary here.
If changing to memcpy(&b[1], &b[0], tail) makes the warning go away, i
am fine with it, but otherwise we should just leave it as is.
> scatterwalk_map_and_copy(b, req->src, offset, tail, 0);
>
> le128_xor(b, &rctx->t, b);
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
2020-07-16 15:56 ` Ard Biesheuvel
@ 2020-07-16 16:05 ` Colin Ian King
2020-07-17 5:21 ` Herbert Xu
1 sibling, 0 replies; 6+ messages in thread
From: Colin Ian King @ 2020-07-16 16:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Herbert Xu, David S . Miller, Linux Crypto Mailing List,
kernel-janitors, Linux Kernel Mailing List
On 16/07/2020 16:56, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 18:29, Colin King <colin.king@canonical.com> wrote:
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There is a memcpy that performs a potential overlapped memory copy
>> from source b to destination b + 1. Fix this by using the safer
>> memmove instead.
>>
>> Addresses-Coverity: ("Overlapping buffer in memory copy")
>> Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> crypto/xts.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/crypto/xts.c b/crypto/xts.c
>> index 3565f3b863a6..fa3e6e7b7043 100644
>> --- a/crypto/xts.c
>> +++ b/crypto/xts.c
>> @@ -169,7 +169,7 @@ static int cts_final(struct skcipher_request *req,
>> offset - XTS_BLOCK_SIZE);
>>
>> scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
>> - memcpy(b + 1, b, tail);
>> + memmove(b + 1, b, tail);
>
> This is a false positive: tail is guaranteed to be smaller than
> sizeof(*b), so memmove() is unnecessary here.
>
> If changing to memcpy(&b[1], &b[0], tail) makes the warning go away, i
> am fine with it, but otherwise we should just leave it as is.
In that case, just leave it as is. Apologies for the noise.
Colin
>
>
>> scatterwalk_map_and_copy(b, req->src, offset, tail, 0);
>>
>> le128_xor(b, &rctx->t, b);
>> --
>> 2.27.0
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
2020-07-16 15:56 ` Ard Biesheuvel
2020-07-16 16:05 ` Colin Ian King
@ 2020-07-17 5:21 ` Herbert Xu
2020-07-17 5:59 ` Ard Biesheuvel
1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2020-07-17 5:21 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Colin King, David S . Miller, Linux Crypto Mailing List,
kernel-janitors, Linux Kernel Mailing List
On Thu, Jul 16, 2020 at 06:56:30PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 18:29, Colin King <colin.king@canonical.com> wrote:
> >
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > There is a memcpy that performs a potential overlapped memory copy
> > from source b to destination b + 1. Fix this by using the safer
> > memmove instead.
> >
> > Addresses-Coverity: ("Overlapping buffer in memory copy")
> > Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > crypto/xts.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/crypto/xts.c b/crypto/xts.c
> > index 3565f3b863a6..fa3e6e7b7043 100644
> > --- a/crypto/xts.c
> > +++ b/crypto/xts.c
> > @@ -169,7 +169,7 @@ static int cts_final(struct skcipher_request *req,
> > offset - XTS_BLOCK_SIZE);
> >
> > scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
> > - memcpy(b + 1, b, tail);
> > + memmove(b + 1, b, tail);
>
> This is a false positive: tail is guaranteed to be smaller than
> sizeof(*b), so memmove() is unnecessary here.
>
> If changing to memcpy(&b[1], &b[0], tail) makes the warning go away, i
> am fine with it, but otherwise we should just leave it as is.
How about a comment perhaps?
Cheers,
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
2020-07-17 5:21 ` Herbert Xu
@ 2020-07-17 5:59 ` Ard Biesheuvel
2020-07-17 6:43 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-07-17 5:59 UTC (permalink / raw)
To: Herbert Xu
Cc: Colin King, David S . Miller, Linux Crypto Mailing List,
kernel-janitors, Linux Kernel Mailing List
On Fri, 17 Jul 2020 at 08:21, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Jul 16, 2020 at 06:56:30PM +0300, Ard Biesheuvel wrote:
> > On Thu, 16 Jul 2020 at 18:29, Colin King <colin.king@canonical.com> wrote:
> > >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > There is a memcpy that performs a potential overlapped memory copy
> > > from source b to destination b + 1. Fix this by using the safer
> > > memmove instead.
> > >
> > > Addresses-Coverity: ("Overlapping buffer in memory copy")
> > > Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > crypto/xts.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/crypto/xts.c b/crypto/xts.c
> > > index 3565f3b863a6..fa3e6e7b7043 100644
> > > --- a/crypto/xts.c
> > > +++ b/crypto/xts.c
> > > @@ -169,7 +169,7 @@ static int cts_final(struct skcipher_request *req,
> > > offset - XTS_BLOCK_SIZE);
> > >
> > > scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
> > > - memcpy(b + 1, b, tail);
> > > + memmove(b + 1, b, tail);
> >
> > This is a false positive: tail is guaranteed to be smaller than
> > sizeof(*b), so memmove() is unnecessary here.
> >
> > If changing to memcpy(&b[1], &b[0], tail) makes the warning go away, i
> > am fine with it, but otherwise we should just leave it as is.
>
> How about a comment perhaps?
>
Or change it to b[1] = b[0] (assuming the compiler allows struct
assignment in that way). This will always copy XTS_BLOCK_SIZE bytes,
but we have sufficient space, and it is probably more efficient too
in most cases.
> Cheers,
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] crypto: xts: use memmove to avoid overlapped memory copy
2020-07-17 5:59 ` Ard Biesheuvel
@ 2020-07-17 6:43 ` Herbert Xu
0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2020-07-17 6:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Colin King, David S . Miller, Linux Crypto Mailing List,
kernel-janitors, Linux Kernel Mailing List
On Fri, Jul 17, 2020 at 08:59:54AM +0300, Ard Biesheuvel wrote:
>
> Or change it to b[1] = b[0] (assuming the compiler allows struct
> assignment in that way). This will always copy XTS_BLOCK_SIZE bytes,
> but we have sufficient space, and it is probably more efficient too
> in most cases.
Sounds good to me.
Thanks,
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-17 6:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 15:29 [PATCH] crypto: xts: use memmove to avoid overlapped memory copy Colin King
2020-07-16 15:56 ` Ard Biesheuvel
2020-07-16 16:05 ` Colin Ian King
2020-07-17 5:21 ` Herbert Xu
2020-07-17 5:59 ` Ard Biesheuvel
2020-07-17 6:43 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).