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.
