| Message ID | 20171123124906.15754-1-omosnacek@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Nov 23, 2017 at 01:49:06PM +0100, Ondrej Mosnacek wrote: > The skcipher_walk_aead_common function calls scatterwalk_copychunks on > the input and output walks to skip the associated data. If the AD end > at an SG list entry boundary, then after these calls the walks will > still be pointing to the end of the skipped region. > > These offsets are later checked for alignment in skcipher_walk_next, > so the skcipher_walk may detect the alignment incorrectly. > > This patch fixes it by calling scatterwalk_done after the copychunks > calls to ensure that the offsets refer to the right SG list entry. > > Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface") > Cc: <stable@vger.kernel.org> > Signed-off-by: Ondrej Mosnacek <omosnacek@gmail.com> Good catch! > diff --git a/crypto/skcipher.c b/crypto/skcipher.c > index 4faa0fd53b0c..6c45ed536664 100644 > --- a/crypto/skcipher.c > +++ b/crypto/skcipher.c > @@ -517,6 +517,9 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, > scatterwalk_copychunks(NULL, &walk->in, req->assoclen, 2); > scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2); > > + scatterwalk_done(&walk->in, 0, walk->total); > + scatterwalk_done(&walk->out, 0, walk->total); That should be 1 instead of 0 for walk->out. Could you please fix and resubmit? Thanks,
(I accidentally hit "reply" instead of "reply all", so resending) 2017-11-24 6:07 GMT+01:00 Herbert Xu <herbert@gondor.apana.org.au>: > On Thu, Nov 23, 2017 at 01:49:06PM +0100, Ondrej Mosnacek wrote: >> diff --git a/crypto/skcipher.c b/crypto/skcipher.c >> index 4faa0fd53b0c..6c45ed536664 100644 >> --- a/crypto/skcipher.c >> +++ b/crypto/skcipher.c >> @@ -517,6 +517,9 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, >> scatterwalk_copychunks(NULL, &walk->in, req->assoclen, 2); >> scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2); >> >> + scatterwalk_done(&walk->in, 0, walk->total); >> + scatterwalk_done(&walk->out, 0, walk->total); > > That should be 1 instead of 0 for walk->out. > > Could you please fix and resubmit? Since the associated data is not written, just skipped, I believe 0 is more appropriate. scatterwalk_copychunks(..., 2) also calls scatterwalk_pagedone() with out=0 internally. O.M.
On Fri, Nov 24, 2017 at 11:53:53AM +0100, Ondrej Mosnáček wrote: > (I accidentally hit "reply" instead of "reply all", so resending) > > 2017-11-24 6:07 GMT+01:00 Herbert Xu <herbert@gondor.apana.org.au>: > > On Thu, Nov 23, 2017 at 01:49:06PM +0100, Ondrej Mosnacek wrote: > >> diff --git a/crypto/skcipher.c b/crypto/skcipher.c > >> index 4faa0fd53b0c..6c45ed536664 100644 > >> --- a/crypto/skcipher.c > >> +++ b/crypto/skcipher.c > >> @@ -517,6 +517,9 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, > >> scatterwalk_copychunks(NULL, &walk->in, req->assoclen, 2); > >> scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2); > >> > >> + scatterwalk_done(&walk->in, 0, walk->total); > >> + scatterwalk_done(&walk->out, 0, walk->total); > > > > That should be 1 instead of 0 for walk->out. > > > > Could you please fix and resubmit? > > Since the associated data is not written, just skipped, I believe 0 is > more appropriate. scatterwalk_copychunks(..., 2) also calls > scatterwalk_pagedone() with out=0 internally. Thanks for the explanation. Patch applied. Cheers,
diff --git a/crypto/skcipher.c b/crypto/skcipher.c index 4faa0fd53b0c..6c45ed536664 100644 --- a/crypto/skcipher.c +++ b/crypto/skcipher.c @@ -517,6 +517,9 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, scatterwalk_copychunks(NULL, &walk->in, req->assoclen, 2); scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2); + scatterwalk_done(&walk->in, 0, walk->total); + scatterwalk_done(&walk->out, 0, walk->total); + walk->iv = req->iv; walk->oiv = req->iv;
The skcipher_walk_aead_common function calls scatterwalk_copychunks on the input and output walks to skip the associated data. If the AD end at an SG list entry boundary, then after these calls the walks will still be pointing to the end of the skipped region. These offsets are later checked for alignment in skcipher_walk_next, so the skcipher_walk may detect the alignment incorrectly. This patch fixes it by calling scatterwalk_done after the copychunks calls to ensure that the offsets refer to the right SG list entry. Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface") Cc: <stable@vger.kernel.org> Signed-off-by: Ondrej Mosnacek <omosnacek@gmail.com> --- crypto/skcipher.c | 3 +++ 1 file changed, 3 insertions(+)