[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Matthias Kaehlcke]

In difference to gas clang doesn't seem to infer the size from the
operands. Add and use the _ASM_MUL macro which determines the operand
size and resolves to the 'mul' instruction with the corresponding
suffix.

This fixes the following error when building with clang:

CC      arch/x86/lib/kaslr.o
/tmp/kaslr-dfe1ad.s: Assembler messages:
/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
no register operands; can't size instruction

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- use _ASM_MUL instead of #ifdef
- updated commit message

 arch/x86/include/asm/asm.h | 1 +
 arch/x86/lib/kaslr.c       | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 7acb51c49fec..7a9df3beb89b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -32,6 +32,7 @@
 #define _ASM_ADD	__ASM_SIZE(add)
 #define _ASM_SUB	__ASM_SIZE(sub)
 #define _ASM_XADD	__ASM_SIZE(xadd)
+#define _ASM_MUL	__ASM_SIZE(mul)
 
 #define _ASM_AX		__ASM_REG(ax)
 #define _ASM_BX		__ASM_REG(bx)
diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
index 121f59c6ee54..0c7fe444dcdd 100644
--- a/arch/x86/lib/kaslr.c
+++ b/arch/x86/lib/kaslr.c
@@ -5,6 +5,7 @@
  * kernel starts. This file is included in the compressed kernel and
  * normally linked in the regular.
  */
+#include <asm/asm.h>
 #include <asm/kaslr.h>
 #include <asm/msr.h>
 #include <asm/archrandom.h>
@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
 	}
 
 	/* Circular multiply for better bit diffusion */
-	asm("mul %3"
+	asm(_ASM_MUL "%3"
 	    : "=a" (random), "=d" (raw)
 	    : "a" (random), "rm" (mix_const));
 	random += raw;
-- 
2.13.0.rc0.306.g87b477812d-goog

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[hpa]

On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>In difference to gas clang doesn't seem to infer the size from the
>operands. Add and use the _ASM_MUL macro which determines the operand
>size and resolves to the 'mul' instruction with the corresponding
>suffix.
>
>This fixes the following error when building with clang:
>
>CC      arch/x86/lib/kaslr.o
>/tmp/kaslr-dfe1ad.s: Assembler messages:
>/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given
>and
>no register operands; can't size instruction
>
>Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>---
>Changes in v2:
>- use _ASM_MUL instead of #ifdef
>- updated commit message
>
> arch/x86/include/asm/asm.h | 1 +
> arch/x86/lib/kaslr.c       | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>index 7acb51c49fec..7a9df3beb89b 100644
>--- a/arch/x86/include/asm/asm.h
>+++ b/arch/x86/include/asm/asm.h
>@@ -32,6 +32,7 @@
> #define _ASM_ADD	__ASM_SIZE(add)
> #define _ASM_SUB	__ASM_SIZE(sub)
> #define _ASM_XADD	__ASM_SIZE(xadd)
>+#define _ASM_MUL	__ASM_SIZE(mul)
> 
> #define _ASM_AX		__ASM_REG(ax)
> #define _ASM_BX		__ASM_REG(bx)
>diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
>index 121f59c6ee54..0c7fe444dcdd 100644
>--- a/arch/x86/lib/kaslr.c
>+++ b/arch/x86/lib/kaslr.c
>@@ -5,6 +5,7 @@
>  * kernel starts. This file is included in the compressed kernel and
>  * normally linked in the regular.
>  */
>+#include <asm/asm.h>
> #include <asm/kaslr.h>
> #include <asm/msr.h>
> #include <asm/archrandom.h>
>@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char
>*purpose)
> 	}
> 
> 	/* Circular multiply for better bit diffusion */
>-	asm("mul %3"
>+	asm(_ASM_MUL "%3"
> 	    : "=a" (random), "=d" (raw)
> 	    : "a" (random), "rm" (mix_const));
> 	random += raw;

This really feels like a "fix your compiler" issue.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Kees Cook]

On Wed, Apr 26, 2017 at 2:00 PM,  <hpa@zytor.com> wrote:
> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>>In difference to gas clang doesn't seem to infer the size from the
>>operands. Add and use the _ASM_MUL macro which determines the operand
>>size and resolves to the 'mul' instruction with the corresponding
>>suffix.
>>
>>This fixes the following error when building with clang:
>>
>>CC      arch/x86/lib/kaslr.o
>>/tmp/kaslr-dfe1ad.s: Assembler messages:
>>/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given
>>and
>>no register operands; can't size instruction
>>
>>Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>---
>>Changes in v2:
>>- use _ASM_MUL instead of #ifdef
>>- updated commit message
>>
>> arch/x86/include/asm/asm.h | 1 +
>> arch/x86/lib/kaslr.c       | 3 ++-
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>>index 7acb51c49fec..7a9df3beb89b 100644
>>--- a/arch/x86/include/asm/asm.h
>>+++ b/arch/x86/include/asm/asm.h
>>@@ -32,6 +32,7 @@
>> #define _ASM_ADD      __ASM_SIZE(add)
>> #define _ASM_SUB      __ASM_SIZE(sub)
>> #define _ASM_XADD     __ASM_SIZE(xadd)
>>+#define _ASM_MUL      __ASM_SIZE(mul)
>>
>> #define _ASM_AX               __ASM_REG(ax)
>> #define _ASM_BX               __ASM_REG(bx)
>>diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
>>index 121f59c6ee54..0c7fe444dcdd 100644
>>--- a/arch/x86/lib/kaslr.c
>>+++ b/arch/x86/lib/kaslr.c
>>@@ -5,6 +5,7 @@
>>  * kernel starts. This file is included in the compressed kernel and
>>  * normally linked in the regular.
>>  */
>>+#include <asm/asm.h>
>> #include <asm/kaslr.h>
>> #include <asm/msr.h>
>> #include <asm/archrandom.h>
>>@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char
>>*purpose)
>>       }
>>
>>       /* Circular multiply for better bit diffusion */
>>-      asm("mul %3"
>>+      asm(_ASM_MUL "%3"
>>           : "=a" (random), "=d" (raw)
>>           : "a" (random), "rm" (mix_const));
>>       random += raw;
>
> This really feels like a "fix your compiler" issue.

We already use the other forms, what's so bad about adding mul too?
And if this lets us build under clang, all the better.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[hpa]

On April 26, 2017 2:21:25 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>On Wed, Apr 26, 2017 at 2:00 PM,  <hpa@zytor.com> wrote:
>> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke
><mka@chromium.org> wrote:
>>>In difference to gas clang doesn't seem to infer the size from the
>>>operands. Add and use the _ASM_MUL macro which determines the operand
>>>size and resolves to the 'mul' instruction with the corresponding
>>>suffix.
>>>
>>>This fixes the following error when building with clang:
>>>
>>>CC      arch/x86/lib/kaslr.o
>>>/tmp/kaslr-dfe1ad.s: Assembler messages:
>>>/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given
>>>and
>>>no register operands; can't size instruction
>>>
>>>Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>---
>>>Changes in v2:
>>>- use _ASM_MUL instead of #ifdef
>>>- updated commit message
>>>
>>> arch/x86/include/asm/asm.h | 1 +
>>> arch/x86/lib/kaslr.c       | 3 ++-
>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>>>index 7acb51c49fec..7a9df3beb89b 100644
>>>--- a/arch/x86/include/asm/asm.h
>>>+++ b/arch/x86/include/asm/asm.h
>>>@@ -32,6 +32,7 @@
>>> #define _ASM_ADD      __ASM_SIZE(add)
>>> #define _ASM_SUB      __ASM_SIZE(sub)
>>> #define _ASM_XADD     __ASM_SIZE(xadd)
>>>+#define _ASM_MUL      __ASM_SIZE(mul)
>>>
>>> #define _ASM_AX               __ASM_REG(ax)
>>> #define _ASM_BX               __ASM_REG(bx)
>>>diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
>>>index 121f59c6ee54..0c7fe444dcdd 100644
>>>--- a/arch/x86/lib/kaslr.c
>>>+++ b/arch/x86/lib/kaslr.c
>>>@@ -5,6 +5,7 @@
>>>  * kernel starts. This file is included in the compressed kernel and
>>>  * normally linked in the regular.
>>>  */
>>>+#include <asm/asm.h>
>>> #include <asm/kaslr.h>
>>> #include <asm/msr.h>
>>> #include <asm/archrandom.h>
>>>@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char
>>>*purpose)
>>>       }
>>>
>>>       /* Circular multiply for better bit diffusion */
>>>-      asm("mul %3"
>>>+      asm(_ASM_MUL "%3"
>>>           : "=a" (random), "=d" (raw)
>>>           : "a" (random), "rm" (mix_const));
>>>       random += raw;
>>
>> This really feels like a "fix your compiler" issue.
>
>We already use the other forms, what's so bad about adding mul too?
>And if this lets us build under clang, all the better.
>
>-Kees

It's not bad per se, but if this doesn't eventually gets fixed in clang we'll have no end of this crap.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Greg Hackmann]

On 04/26/2017 02:24 PM, hpa@zytor.com wrote:
>>> This really feels like a "fix your compiler" issue.
>>
>> We already use the other forms, what's so bad about adding mul too?
>> And if this lets us build under clang, all the better.
>>
>> -Kees
>
> It's not bad per se, but if this doesn't eventually gets fixed in clang we'll have no end of this crap.
>

AIUI the "problem" is that clang is spilling mix_const into memory 
rather than assigning it to a register.  This is perfectly legal since 
mix_const has a constraint of "rm".  But mul needs a suffix when the 
input is a memory location, since it can't infer the multiplication 
width from the input operand anymore.

You get the same error message with gcc if you force it to use a memory 
location, by narrowing the constraint from "rm" to "m".

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Kees Cook]

On Wed, Apr 26, 2017 at 1:55 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> In difference to gas clang doesn't seem to infer the size from the
> operands. Add and use the _ASM_MUL macro which determines the operand
> size and resolves to the 'mul' instruction with the corresponding
> suffix.
>
> This fixes the following error when building with clang:
>
> CC      arch/x86/lib/kaslr.o
> /tmp/kaslr-dfe1ad.s: Assembler messages:
> /tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
> no register operands; can't size instruction
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Changes in v2:
> - use _ASM_MUL instead of #ifdef
> - updated commit message
>
>  arch/x86/include/asm/asm.h | 1 +
>  arch/x86/lib/kaslr.c       | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 7acb51c49fec..7a9df3beb89b 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -32,6 +32,7 @@
>  #define _ASM_ADD       __ASM_SIZE(add)
>  #define _ASM_SUB       __ASM_SIZE(sub)
>  #define _ASM_XADD      __ASM_SIZE(xadd)
> +#define _ASM_MUL       __ASM_SIZE(mul)
>
>  #define _ASM_AX                __ASM_REG(ax)
>  #define _ASM_BX                __ASM_REG(bx)
> diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
> index 121f59c6ee54..0c7fe444dcdd 100644
> --- a/arch/x86/lib/kaslr.c
> +++ b/arch/x86/lib/kaslr.c
> @@ -5,6 +5,7 @@
>   * kernel starts. This file is included in the compressed kernel and
>   * normally linked in the regular.
>   */
> +#include <asm/asm.h>
>  #include <asm/kaslr.h>
>  #include <asm/msr.h>
>  #include <asm/archrandom.h>
> @@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
>         }
>
>         /* Circular multiply for better bit diffusion */
> -       asm("mul %3"
> +       asm(_ASM_MUL "%3"
>             : "=a" (random), "=d" (raw)
>             : "a" (random), "rm" (mix_const));
>         random += raw;
> --
> 2.13.0.rc0.306.g87b477812d-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[H. Peter Anvin]

On 04/26/17 14:29, Greg Hackmann wrote:
> On 04/26/2017 02:24 PM, hpa@zytor.com wrote:
>>>> This really feels like a "fix your compiler" issue.
>>>
>>> We already use the other forms, what's so bad about adding mul too?
>>> And if this lets us build under clang, all the better.
>>>
>>> -Kees
>>
>> It's not bad per se, but if this doesn't eventually gets fixed in
>> clang we'll have no end of this crap.
>>
> 
> AIUI the "problem" is that clang is spilling mix_const into memory
> rather than assigning it to a register.  This is perfectly legal since
> mix_const has a constraint of "rm".  But mul needs a suffix when the
> input is a memory location, since it can't infer the multiplication
> width from the input operand anymore.
> 
> You get the same error message with gcc if you force it to use a memory
> location, by narrowing the constraint from "rm" to "m".

OK, that's a genuine bug.  Please explain that in the comment; it has
nothing to do with clang.

	-hpa

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Kees Cook]

On Mon, May 1, 2017 at 3:47 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The constraint "rm" allows the compiler to put mix_const into memory.
> When the input operand is a memory location mul needs an operand size
> suffix, since it can't infer the multiplication width from the operand.
>
> Add and use the _ASM_MUL macro which determines the operand size and
> resolves to the 'mul' instruction with the corresponding suffix.
>
> This fixes the following error when building with clang:
>
> CC      arch/x86/lib/kaslr.o
> /tmp/kaslr-dfe1ad.s: Assembler messages:
> /tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
> no register operands; can't size instruction
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: Kees Cook <keescook@chromium.org>

Thanks for the v2!

-Kees

> ---
> Changes in v2:
> - updated commit message with additional details
>
>  arch/x86/include/asm/asm.h | 1 +
>  arch/x86/lib/kaslr.c       | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 7acb51c49fec..7a9df3beb89b 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -32,6 +32,7 @@
>  #define _ASM_ADD       __ASM_SIZE(add)
>  #define _ASM_SUB       __ASM_SIZE(sub)
>  #define _ASM_XADD      __ASM_SIZE(xadd)
> +#define _ASM_MUL       __ASM_SIZE(mul)
>
>  #define _ASM_AX                __ASM_REG(ax)
>  #define _ASM_BX                __ASM_REG(bx)
> diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
> index 121f59c6ee54..0c7fe444dcdd 100644
> --- a/arch/x86/lib/kaslr.c
> +++ b/arch/x86/lib/kaslr.c
> @@ -5,6 +5,7 @@
>   * kernel starts. This file is included in the compressed kernel and
>   * normally linked in the regular.
>   */
> +#include <asm/asm.h>
>  #include <asm/kaslr.h>
>  #include <asm/msr.h>
>  #include <asm/archrandom.h>
> @@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
>         }
>
>         /* Circular multiply for better bit diffusion */
> -       asm("mul %3"
> +       asm(_ASM_MUL "%3"
>             : "=a" (random), "=d" (raw)
>             : "a" (random), "rm" (mix_const));
>         random += raw;
> --
> 2.13.0.rc1.294.g07d810a77f-goog
>



-- 
Kees Cook
Pixel Security

[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Matthias Kaehlcke]

The constraint "rm" allows the compiler to put mix_const into memory.
When the input operand is a memory location mul needs an operand size
suffix, since it can't infer the multiplication width from the operand.

Add and use the _ASM_MUL macro which determines the operand size and
resolves to the 'mul' instruction with the corresponding suffix.

This fixes the following error when building with clang:

CC      arch/x86/lib/kaslr.o
/tmp/kaslr-dfe1ad.s: Assembler messages:
/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
no register operands; can't size instruction

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- updated commit message with additional details

 arch/x86/include/asm/asm.h | 1 +
 arch/x86/lib/kaslr.c       | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 7acb51c49fec..7a9df3beb89b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -32,6 +32,7 @@
 #define _ASM_ADD	__ASM_SIZE(add)
 #define _ASM_SUB	__ASM_SIZE(sub)
 #define _ASM_XADD	__ASM_SIZE(xadd)
+#define _ASM_MUL	__ASM_SIZE(mul)
 
 #define _ASM_AX		__ASM_REG(ax)
 #define _ASM_BX		__ASM_REG(bx)
diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
index 121f59c6ee54..0c7fe444dcdd 100644
--- a/arch/x86/lib/kaslr.c
+++ b/arch/x86/lib/kaslr.c
@@ -5,6 +5,7 @@
  * kernel starts. This file is included in the compressed kernel and
  * normally linked in the regular.
  */
+#include <asm/asm.h>
 #include <asm/kaslr.h>
 #include <asm/msr.h>
 #include <asm/archrandom.h>
@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
 	}
 
 	/* Circular multiply for better bit diffusion */
-	asm("mul %3"
+	asm(_ASM_MUL "%3"
 	    : "=a" (random), "=d" (raw)
 	    : "a" (random), "rm" (mix_const));
 	random += raw;
-- 
2.13.0.rc1.294.g07d810a77f-goog