On Mon, May 22, 2017 at 09:39:57PM +0200, Mike Belopuhov wrote:
> On Mon, May 22, 2017 at 12:34 -0700, Mike Larkin wrote:
> > On Mon, May 22, 2017 at 08:55:31PM +0200, Mike Belopuhov wrote:
> > > On Mon, May 22, 2017 at 20:20 +0200, Sebastien Marie wrote:
> > > > On Mon, May 22, 2017 at 07:32:07PM +0200, Mike Belopuhov wrote:
> > > > > >
> > > > > > With the last commit to revert AES_XTS to rijndael, I pushed it on
> > > > > > top of the tested tree (7 days old). The hibernate/resume works
> > > > > > again.
> > > > > >
> > > > > > It makes it to confirm the problem is related to the switch to
> > > > > > constant-time-aes in the context of full-disk-encryption.
> > > > > >
> > > > >
> > > > > Thanks for verifying this. I've looked through the sr_hibernate_io
> > > > > (that's hib->io_func) but couldn't find anything wrong with it. The
> > > > > only thing that springs to mind is that AES_CTX and therefore the
> > > > > XTS context (aes_xts_ctx) is larger and requires more stack space.
> > > > > Though I can't see what might be affected by that.
> > > >
> > > > I quickly check the difference of struct size and come back with some
> > > > idea to test.
> > > >
> > > > - aes_xts_ctx based on AES_CTX has sizeof()=1464
> > > > - aes_xts_ctx based on rijndael_ctx has sizeof()=992
> > > >
> > > >
> > > > do you think just adding padding of 472 (1464-992) at end of `struct
> > > > aes_xts_ctx' (rijndael_ctx version) would be enough to test it ?
> > > >
> > > > Something like:
> > > >
> > > > diff --git a/sys/crypto/xform.c b/sys/crypto/xform.c
> > > > index 71e173b44..9775d030c 100644
> > > > --- a/sys/crypto/xform.c
> > > > +++ b/sys/crypto/xform.c
> > > > @@ -125,6 +125,7 @@ struct aes_xts_ctx {
> > > > rijndael_ctx key1;
> > > > rijndael_ctx key2;
> > > > u_int8_t tweak[AES_XTS_BLOCKSIZE];
> > > > + u_int8_t _pad[472];
> > > > };
> > > >
> > > > /* Helper */
> > > >
> > >
> > > Good question. I would try both in the end of the structure and in
> > > the beginning, in attempt to at least rule out stack corruption.
> > >
> > > >
> > > > if the hibernate/resume process is able to complete with the padded
> > > > version, we could exclude the struct size of the equation. and if the
> > > > process fail, it means the size matters.
> > > >
> > >
> > > I would agree with this.
> > >
> > > > any additional thing to do for testing padding (initialisation or
> > > > something else) ? you know better than me this subsystem :)
> > > >
> > >
> > > Nothing springs to mind, but I didn't have time to investigate
> > > any further yet.
> > >
> >
> > Is the new AES implementation side effect free? It cannot touch any memory
> > outside the scratch page used by the sr crypto suspend routine.
> >
> > -ml
>
> It doesn't touch any memory except for its context which is on the stack.
> If this stack must fit into the scratch page, I don't see any indication
> of enforcing it or a comment explaining it. aes_xts_ctx is not part of
> the "*my = page;" structure.
>
A private stack is in use while hibernate is occurring, so as long as the stack
doesn't grow past 3 pages it should be ok. The *my = page is for any extra
working area the I/O routines need.
-ml
