[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.