| Message ID | 20190821143253.30209-9-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: arm/aes - XTS ciphertext stealing and other updates | expand |
On Wed, Aug 21, 2019 at 05:32:44PM +0300, Ard Biesheuvel wrote: > After starting a skcipher walk, the only way to ensure that all > resources it has tied up are released is to complete it. In some > cases, it will be useful to be able to abort a walk cleanly after > it has started, so add this ability to the skcipher walk API. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > crypto/skcipher.c | 3 +++ > include/crypto/internal/skcipher.h | 5 +++++ > 2 files changed, 8 insertions(+) > > diff --git a/crypto/skcipher.c b/crypto/skcipher.c > index 5d836fc3df3e..973ab1c7dcca 100644 > --- a/crypto/skcipher.c > +++ b/crypto/skcipher.c > @@ -140,6 +140,9 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) > goto already_advanced; > } > > + if (unlikely(!n)) > + goto finish; > + > scatterwalk_advance(&walk->in, n); > scatterwalk_advance(&walk->out, n); > already_advanced: > diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h > index d68faa5759ad..bc488173531f 100644 > --- a/include/crypto/internal/skcipher.h > +++ b/include/crypto/internal/skcipher.h > @@ -148,6 +148,11 @@ int skcipher_walk_aead_decrypt(struct skcipher_walk *walk, > struct aead_request *req, bool atomic); > void skcipher_walk_complete(struct skcipher_walk *walk, int err); > > +static inline void skcipher_walk_abort(struct skcipher_walk *walk) > +{ > + skcipher_walk_done(walk, walk->nbytes); > +} Couldn't you just abort it by supplying an error in place of walk->bytes? IOW I'm fine with this helper but you don't need to touch skcipher_walk_done as just giving it an negative err value should do the trick. 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
On Fri, 30 Aug 2019 at 11:03, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Wed, Aug 21, 2019 at 05:32:44PM +0300, Ard Biesheuvel wrote: > > After starting a skcipher walk, the only way to ensure that all > > resources it has tied up are released is to complete it. In some > > cases, it will be useful to be able to abort a walk cleanly after > > it has started, so add this ability to the skcipher walk API. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > crypto/skcipher.c | 3 +++ > > include/crypto/internal/skcipher.h | 5 +++++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/crypto/skcipher.c b/crypto/skcipher.c > > index 5d836fc3df3e..973ab1c7dcca 100644 > > --- a/crypto/skcipher.c > > +++ b/crypto/skcipher.c > > @@ -140,6 +140,9 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) > > goto already_advanced; > > } > > > > + if (unlikely(!n)) > > + goto finish; > > + > > scatterwalk_advance(&walk->in, n); > > scatterwalk_advance(&walk->out, n); > > already_advanced: > > diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h > > index d68faa5759ad..bc488173531f 100644 > > --- a/include/crypto/internal/skcipher.h > > +++ b/include/crypto/internal/skcipher.h > > @@ -148,6 +148,11 @@ int skcipher_walk_aead_decrypt(struct skcipher_walk *walk, > > struct aead_request *req, bool atomic); > > void skcipher_walk_complete(struct skcipher_walk *walk, int err); > > > > +static inline void skcipher_walk_abort(struct skcipher_walk *walk) > > +{ > > + skcipher_walk_done(walk, walk->nbytes); > > +} > > Couldn't you just abort it by supplying an error in place of > walk->bytes? IOW I'm fine with this helper but you don't need > to touch skcipher_walk_done as just giving it an negative err > value should do the trick. > This might be a problem with the implementation of skcipher_walk_done() in general rather than a limitation in this particular case, but when calling skcipher_walk_done() with a negative err value, we never kunmap the src and dst pages. So should I propose a fix for that instead? Or are the internal callers dealing with this correctly? (and is it forbidden for external callers to pass negative values?)
On Tue, Sep 03, 2019 at 04:54:38PM +1000, Herbert Xu wrote: > int skcipher_walk_done(struct skcipher_walk *walk, int err) > { > - unsigned int n; /* bytes processed */ > - bool more; > - > - if (unlikely(err < 0)) > - goto finish; > + unsigned int n = walk->nbytes - err; > + unsigned int nbytes; > > - n = walk->nbytes - err; > - walk->total -= n; > - more = (walk->total != 0); > + nbytes = walk->total - n; > > - if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS | > - SKCIPHER_WALK_SLOW | > - SKCIPHER_WALK_COPY | > - SKCIPHER_WALK_DIFF)))) { > + if (unlikely(err < 0)) { > + nbytes = 0; > + n = 0; > + } else if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS | > + SKCIPHER_WALK_SLOW | > + SKCIPHER_WALK_COPY | > + SKCIPHER_WALK_DIFF)))) { > unmap_src: > skcipher_unmap_src(walk); > } else if (walk->flags & SKCIPHER_WALK_DIFF) { > @@ -134,25 +134,34 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) > * the algorithm requires it. > */ > err = -EINVAL; > - goto finish; > - } > - skcipher_done_slow(walk, n); > - goto already_advanced; > + nbytes = 0; > + } else > + n = skcipher_done_slow(walk, n); > } > > + if (err > 0) > + err = 0; > + > + walk->total = nbytes; > + walk->nbytes = nbytes; > + > scatterwalk_advance(&walk->in, n); > scatterwalk_advance(&walk->out, n); > -already_advanced: > - scatterwalk_done(&walk->in, 0, more); > - scatterwalk_done(&walk->out, 1, more); > + scatterwalk_done(&walk->in, 0, nbytes); > + scatterwalk_done(&walk->out, 1, nbytes); > > - if (more) { > + if (nbytes) { > crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ? > CRYPTO_TFM_REQ_MAY_SLEEP : 0); > return skcipher_walk_next(walk); > } > - err = 0; > -finish: > + > + return skcipher_walk_unwind(walk, err); > +} > +EXPORT_SYMBOL_GPL(skcipher_walk_done); Doesn't this re-introduce the same bug that my patch fixed -- that scatterwalk_done() could be called after 0 bytes processed, causing a crash in scatterwalk_pagedone()? - Eric
On Tue, Sep 03, 2019 at 08:50:20AM -0500, Eric Biggers wrote: > > Doesn't this re-introduce the same bug that my patch fixed -- that > scatterwalk_done() could be called after 0 bytes processed, causing a crash in > scatterwalk_pagedone()? No because that crash is caused by the internal calls to the function skcipher_walk_done with an error. Those two internal calls have now been changed into skcipher_walk_unwind which does not try to unmap the pages. 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
On Wed, Sep 04, 2019 at 08:36:41AM +1000, Herbert Xu wrote: > On Tue, Sep 03, 2019 at 08:50:20AM -0500, Eric Biggers wrote: > > > > Doesn't this re-introduce the same bug that my patch fixed -- that > > scatterwalk_done() could be called after 0 bytes processed, causing a crash in > > scatterwalk_pagedone()? > > No because that crash is caused by the internal calls to the > function skcipher_walk_done with an error. Those two internal > calls have now been changed into skcipher_walk_unwind which does > not try to unmap the pages. > Okay, but what about external callers that pass in an error? (I mean, I don't actually see any currently, but the point of this patch is to allow it...) What would prevent the crash in scatterwalk_done() in that case? - Eric
On Wed, Sep 04, 2019 at 10:22:17PM -0700, Eric Biggers wrote: > > Okay, but what about external callers that pass in an error? (I mean, I don't > actually see any currently, but the point of this patch is to allow it...) > What would prevent the crash in scatterwalk_done() in that case? With external callers the pages are always mapped and therefore they have to be unmapped, regardless of whether the actual crypto succeeded or not. 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
On Thu, Sep 05, 2019 at 03:40:32PM +1000, Herbert Xu wrote: > On Wed, Sep 04, 2019 at 10:22:17PM -0700, Eric Biggers wrote: > > > > Okay, but what about external callers that pass in an error? (I mean, I don't > > actually see any currently, but the point of this patch is to allow it...) > > What would prevent the crash in scatterwalk_done() in that case? > > With external callers the pages are always mapped and therefore > they have to be unmapped, regardless of whether the actual crypto > succeeded or not. > That's not what I'm talking about. I'm talking about flushing the page, in scatterwalk_done(). It assumes the page that was just processed was: sg_page(walk->sg) + ((walk->offset - 1) >> PAGE_SHIFT) But if no bytes were processed, this is invalid. Notably, if no bytes were processed then walk->offset can be 0, causing a crash. - Eric
On Thu, Sep 05, 2019 at 06:57:53PM -0700, Eric Biggers wrote: > > That's not what I'm talking about. I'm talking about flushing the page, in > scatterwalk_done(). It assumes the page that was just processed was: > > sg_page(walk->sg) + ((walk->offset - 1) >> PAGE_SHIFT) > > But if no bytes were processed, this is invalid. Notably, if no bytes were > processed then walk->offset can be 0, causing a crash. You're right. What's worse is that my patch doesn't even unmap the pages anyway. Let me do this again. 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
On Thu, 5 Sep 2019 at 20:13, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > skcipher_walk_done may be called with an error by internal or > external callers. For those internal callers we shouldn't unmap > pages but for external callers we must unmap any pages that are > in use. > > This patch distinguishes between the two cases by checking whether > walk->nbytes is zero or not. For internal callers, we now set > walk->nbytes to zero prior to the call. For external callers, > walk->nbytes has always been non-zero (as zero is used to indicate > the termination of a walk). > > Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Fixes: 5cde0af2a982 ("[CRYPTO] cipher: Added block cipher type") > Cc: <stable@vger.kernel.org> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/crypto/skcipher.c b/crypto/skcipher.c > index 5d836fc3df3e..22753c1c7202 100644 > --- a/crypto/skcipher.c > +++ b/crypto/skcipher.c > @@ -90,7 +90,7 @@ static inline u8 *skcipher_get_spot(u8 *start, unsigned int len) > return max(start, end_page); > } > > -static void skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize) > +static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize) > { > u8 *addr; > > @@ -98,19 +98,21 @@ static void skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize) > addr = skcipher_get_spot(addr, bsize); > scatterwalk_copychunks(addr, &walk->out, bsize, > (walk->flags & SKCIPHER_WALK_PHYS) ? 2 : 1); > + return 0; > } > > int skcipher_walk_done(struct skcipher_walk *walk, int err) > { > - unsigned int n; /* bytes processed */ > - bool more; > + unsigned int n = walk->nbytes; > + unsigned int nbytes = 0; > > - if (unlikely(err < 0)) > + if (!n) > goto finish; > > - n = walk->nbytes - err; > - walk->total -= n; > - more = (walk->total != 0); > + if (likely(err >= 0)) { > + n -= err; > + nbytes = walk->total - n; > + } > > if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS | > SKCIPHER_WALK_SLOW | With this change, we still copy out the output in the SKCIPHER_WALK_COPY or SKCIPHER_WALK_SLOW cases. I'd expect the failure case to only do the kunmap()s, but otherwise not make any changes that are visible to the caller. > @@ -126,7 +128,7 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) > memcpy(walk->dst.virt.addr, walk->page, n); > skcipher_unmap_dst(walk); > } else if (unlikely(walk->flags & SKCIPHER_WALK_SLOW)) { > - if (err) { > + if (err > 0) { > /* > * Didn't process all bytes. Either the algorithm is > * broken, or this was the last step and it turned out > @@ -134,27 +136,29 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) > * the algorithm requires it. > */ > err = -EINVAL; > - goto finish; > - } > - skcipher_done_slow(walk, n); > - goto already_advanced; > + nbytes = 0; > + } else > + n = skcipher_done_slow(walk, n); > } > > + if (err > 0) > + err = 0; > + > + walk->total = nbytes; > + walk->nbytes = 0; > + > scatterwalk_advance(&walk->in, n); > scatterwalk_advance(&walk->out, n); > -already_advanced: > - scatterwalk_done(&walk->in, 0, more); > - scatterwalk_done(&walk->out, 1, more); > + scatterwalk_done(&walk->in, 0, nbytes); > + scatterwalk_done(&walk->out, 1, nbytes); > > - if (more) { > + if (nbytes) { > crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ? > CRYPTO_TFM_REQ_MAY_SLEEP : 0); > return skcipher_walk_next(walk); > } > - err = 0; > -finish: > - walk->nbytes = 0; > > +finish: > /* Short-circuit for the common/fast path. */ > if (!((unsigned long)walk->buffer | (unsigned long)walk->page)) > goto out; > -- > 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
On Fri, Sep 06, 2019 at 05:52:56PM -0700, Ard Biesheuvel wrote: > > With this change, we still copy out the output in the > SKCIPHER_WALK_COPY or SKCIPHER_WALK_SLOW cases. I'd expect the failure > case to only do the kunmap()s, but otherwise not make any changes that > are visible to the caller. I don't think it matters. After all, for the fast/common path whatever changes that have been made will be visible to the caller. I don't see the point in making the slow-path different in this respect. It also makes no sense to optimise specifically for the uncommon error case on the slow-path. 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
On Fri, 6 Sep 2019 at 18:19, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Fri, Sep 06, 2019 at 05:52:56PM -0700, Ard Biesheuvel wrote: > > > > With this change, we still copy out the output in the > > SKCIPHER_WALK_COPY or SKCIPHER_WALK_SLOW cases. I'd expect the failure > > case to only do the kunmap()s, but otherwise not make any changes that > > are visible to the caller. > > I don't think it matters. After all, for the fast/common path > whatever changes that have been made will be visible to the caller. > I don't see the point in making the slow-path different in this > respect. It also makes no sense to optimise specifically for the > uncommon error case on the slow-path. > The point is that doing skcipher_walk_virt(&walk, ...); skcipher_walk_done(&walk, -EFOO); may clobber your data if you are executing in place (unless I am missing something) If skcipher_walk_done() is called with an error, it should really just clean up after it self, but not copy back the unknown contents of temporary buffers.
On Fri, Sep 06, 2019 at 06:32:29PM -0700, Ard Biesheuvel wrote: > > The point is that doing > > skcipher_walk_virt(&walk, ...); > skcipher_walk_done(&walk, -EFOO); > > may clobber your data if you are executing in place (unless I am > missing something) You mean encrypting in place? If you're encrypting in place you're usually on the zero-copy fast path so whatever is left-behind by the algorithm will be visible anyway without any copying. > If skcipher_walk_done() is called with an error, it should really just > clean up after it self, but not copy back the unknown contents of > temporary buffers. We're not copying uninitialised kernel memory. The temporary space starts out as a copy of the source and we're just copying it to the destination. 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
On Fri, 6 Sep 2019 at 18:56, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Fri, Sep 06, 2019 at 06:32:29PM -0700, Ard Biesheuvel wrote: > > > > The point is that doing > > > > skcipher_walk_virt(&walk, ...); > > skcipher_walk_done(&walk, -EFOO); > > > > may clobber your data if you are executing in place (unless I am > > missing something) > > You mean encrypting in place? If you're encrypting in place you're > usually on the zero-copy fast path so whatever is left-behind by the > algorithm will be visible anyway without any copying. > > > If skcipher_walk_done() is called with an error, it should really just > > clean up after it self, but not copy back the unknown contents of > > temporary buffers. > > We're not copying uninitialised kernel memory. The temporary space > starts out as a copy of the source and we're just copying it to the > destination. > Right. In that case, I guess it is safe. I've tested my XTS/CTS changes (which call skcipher_walk_done() with an error value in some cases) with Eric's fuzz testing enabled, and it all works fine, so Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thanks, Ard.
diff --git a/crypto/skcipher.c b/crypto/skcipher.c index 5d836fc3df3e..973ab1c7dcca 100644 --- a/crypto/skcipher.c +++ b/crypto/skcipher.c @@ -140,6 +140,9 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err) goto already_advanced; } + if (unlikely(!n)) + goto finish; + scatterwalk_advance(&walk->in, n); scatterwalk_advance(&walk->out, n); already_advanced: diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h index d68faa5759ad..bc488173531f 100644 --- a/include/crypto/internal/skcipher.h +++ b/include/crypto/internal/skcipher.h @@ -148,6 +148,11 @@ int skcipher_walk_aead_decrypt(struct skcipher_walk *walk, struct aead_request *req, bool atomic); void skcipher_walk_complete(struct skcipher_walk *walk, int err); +static inline void skcipher_walk_abort(struct skcipher_walk *walk) +{ + skcipher_walk_done(walk, walk->nbytes); +} + static inline void ablkcipher_request_complete(struct ablkcipher_request *req, int err) {
After starting a skcipher walk, the only way to ensure that all resources it has tied up are released is to complete it. In some cases, it will be useful to be able to abort a walk cleanly after it has started, so add this ability to the skcipher walk API. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- crypto/skcipher.c | 3 +++ include/crypto/internal/skcipher.h | 5 +++++ 2 files changed, 8 insertions(+) -- 2.17.1