<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=windows-1252">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 10/12/2015 19:13, Benjamin Kaduk
      wrote:<br>
    </div>
    <blockquote class=" cite" id="mid_5669C0B9_10901_akamai_com"
      cite="mid:5669C0B9.10901@akamai.com" type="cite">
      <pre wrap="">On 12/10/2015 12:09 PM, <a class="moz-txt-link-abbreviated" href="mailto:openssl-users@dukhovni.org">openssl-users@dukhovni.org</a> wrote:
</pre>
      <blockquote class=" cite" id="Cite_3573417" type="cite">
        <blockquote class=" cite" id="Cite_4741901" type="cite">
          <pre wrap="">On Dec 10, 2015, at 12:45 PM, Jakob Bohm <a class="moz-txt-link-rfc2396E" href="mailto:jb-openssl@wisemo.com"><jb-openssl@wisemo.com></a> wrote:

On 10/12/2015 18:33, Viktor Dukhovni wrote:
</pre>
          <blockquote class=" cite" id="Cite_7028633" type="cite">
            <pre wrap="">On Thu, Dec 10, 2015 at 04:55:29AM -0700, Jayalakshmi bhat wrote:


</pre>
            <blockquote class=" cite" id="Cite_1984145" type="cite">
              <pre wrap="">static inline unsigned int constant_time_msb(unsigned int a) {
-  return 0 - (a >> (sizeof(a) * 8 - 1));
+ return (((unsigned)((int)(a) >> (sizeof(int) * 8 - 1))));
}

</pre>
            </blockquote>
            <pre wrap="">The replacement is not right.  This function is supposed to return
0xfffffff for inputs with the high bit set, and 0x0000000 for inputs
with the high bit not set.  Could you try:

    static inline unsigned int constant_time_msb(unsigned int a) {
      return 0 - (a >> ((int)(sizeof(a) * 8 - 1)));
    }

Just in case the compiler is promoting "a" to the (larger?) size
of sizeof(a), which would cause an unsigned "a" to get a zero MSB,
while a signed "a" would be promoted "correctly".

</pre>
          </blockquote>
          <pre wrap="">Look again, he is casting a to signed, then doing an 
arithmetic right shift to extend the msb (sign bit) 
to the rest of the word.  This works on 3 conditions:
</pre>
        </blockquote>
        <pre wrap="">I saw what he's doing.  casting (a) to an int, instead of leaving
it unsigned is not an improvement.  On the assumption that it made
a difference for this compiler, I proposed an alternative that tests
whether promotion to the type of the shift's right operand is taking
place.  It would be good to know whether explicitly casting the shift
width to an int helps.  Also that 8 could be replaced by CHAR_BIT
just in case.

</pre>
      </blockquote>
      <pre wrap="">Yeah, sizeof returning a size_t that is wider than int, causing
promotion of the left argument of the shift operator prior to evaluation
seems a plausible explanation for this behavior.
</pre>
    </blockquote>
    <tt>Please stop the misinformed guesswork and read the <br>
      disassembly posted.<br>
      <br>
      The compiler in question gets confused by the long <br>
      sequence of nested inline expressions and looses the <br>
      truncation from 32 bit unsigned to 8 bit unsigned <br>
      char in the shuffle.<br>
      <br>
      Changing back to the old form of constant_time_msb <br>
      simplifies the parse tree just enough to avoid the bug.<br>
      <br>
      Unfortunately, as a comment in the 1.0.2 source code <br>
      explains, the old form relies on a language feature <br>
      not guaranteed by the C standard, which is probably <br>
      why the implementation was changed to the one <br>
      currently in 1.0.2.<br>
      <br>
      And to those who still don't understand how the old <br>
      implementation worked:<br>
      <br>
      1. By casting the unsigned a to a signed int, the msb <br>
        of interest becomes the sign bit.<br>
      <br>
      2. By shifting right the 2's complement signed int by <br>
        the number of non-sign bits (31 on this target), <br>
        the sign bit gets replicated to the other bits so <br>
        negative values (those with the msb set) become -1, <br>
        while positive values (those with the msb clear) <br>
        become +0.<br>
      <br>
      3. Casting back to unsigned int results in the <br>
        desired value of 0xFFFFFFFF or 0x00000000 .<br>
      <br>
      On the ARM platform, shifting right by 31 bits can be <br>
      done to the second input of any arithmetic <br>
      instruction, thus making it execute in 0.5 instruction <br>
      time if combined with some other operation.  Thus the <br>
      compiler moves the shift backwards through some <br>
      arithmetic steps in the zero testing, resulting in the <br>
      code you see in the posted disassembly.<br>
      <br>
      <br>
      <br>
    </tt>
    <pre class="moz-signature" cols="72">Enjoy

Jakob
-- 
Jakob Bohm, CIO, Partner, WiseMo A/S.  <a class="moz-txt-link-freetext" href="https://www.wisemo.com">https://www.wisemo.com</a>
Transformervej 29, 2860 Søborg, Denmark.  Direct +45 31 13 16 10
This public discussion message is non-binding and may contain errors.
WiseMo - Remote Service Management for PCs, Phones and Embedded </pre>
  </body>
</html>