Re: [Xen-devel] [PATCH] vtpmmgr: fix 32-bit compilation

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 04/25/2014 03:13 AM, Jan Beulich wrote:
> > > > On 24.04.14 at 22:39, <dgdegra@xxxxxxxxxxxxx> wrote:
> > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c
> > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> > @@ -451,6 +451,7 @@ static TPM_RESULT vtpmmgr_GroupActivate(tpmcmd_t* tpmcmd)
> >    * mpi objects use little endian word ordering
> >    */
> >   static t_uint Pp[256 / sizeof(t_uint)] = {
> > +#ifdef __x86_64__
> >         0xFFFFFFFFFFFFFFFFUL, 0x15728E5A8AACAA68UL, 0x15D2261898FA0510UL,
> >         0x3995497CEA956AE5UL, 0xDE2BCBF695581718UL, 0xB5C55DF06F4C52C9UL,
> >         0x9B2783A2EC07A28FUL, 0xE39E772C180E8603UL, 0x32905E462E36CE3BUL,
> Isn't looking for just a single architecture a little too specific here?
> What about other 64-bit architectures, namely aarch64?
[...]
> And the resulting redundancy doesn't seem desirable either. Perhaps
> this ought to be initialized as an array of bytes?
The proper way to do this would be to store it as a big-endian byte
array and then import it to a dynamically allocated MPI object.  I was
trying to be more efficient and avoid byte order conversions on
constants, but it sounds like it would be better to just do it the slow
way.  This isn't a time- or memory-critical operation so there's not a
big reason to try to be efficient.

> > @@ -469,15 +483,16 @@ static void tm_dhkx_gen(void* dhkx1, void* dhkx2, void* 
> > out)
> >   {
> >         mpi GX = { 0 }, GY = { 0 }, K = { 0 }, RP = { 0 };
> >
> > -       t_uint Xp[256 / sizeof(t_uint)];
> > +       int XpElts = 256 / sizeof(t_uint);
> If the two arrays need to be the same size (not sure whether that's
> the case), an equivalent of ARRAY_SIZE(Pp) (open coded in the
> current code as seen below) would seem to be the better alternative.
If P is populated from a byte array, then this value (which does need
to be the same size as P) will still need to be created by division.

> > +       t_uint Xp[XpElts];
> Or maybe simply typeof(Pp) here, ...
No, the type is dictated by the mpi object, not by Pp.  If using typeof,
it should be typeof(X.n), although X isn't declared yet so that exact
expression isn't valid.

> >         mpi X = {
> >                 .s = 1,
> > -               .n = sizeof(Xp)/sizeof(Xp[0]),
> > +               .n = XpElts,
> >                 .p = Xp
> >         };
> >         mpi P = {
> >                 .s = 1,
> > -               .n = sizeof(Pp)/sizeof(Pp[0]),
> > +               .n = XpElts,
> ... and these two left as they were?
>
> > @@ -487,8 +502,8 @@ static void tm_dhkx_gen(void* dhkx1, void* dhkx2, void* out)
> >         };
> >
> >         do_random(Xp, sizeof(Xp));
> > -       while (Xp[31] == 0 || Xp[31] == -1UL)
> > -               do_random(Xp + 31, sizeof(Xp[31]));
> > +       while (Xp[XpElts - 1] == 0 || Xp[XpElts - 1] == -1UL)
> Wouldn't that better be ~(t_uint)0, or - without any cast -
> !(Xp[XpElts - 1] + 1)?
True, but both of those were longer and (at least imo) no more clear.

> > +               do_random(Xp + XpElts - 1, sizeof(Xp[0]));
> Also it looks odd to me that this deals with a different number of
> trailing bit depending on architecture - is that really correct?
>
> Jan
This check is probably not needed in practice: it handles the 1 in 2^66
chance that the random number is outside the MODP group by discarding
either 1/2^31 or 1/2^63 possibilities (also discarding low exponents).
I suppose a comment explaining this would be useful.

--
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel