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 <[email protected]>
---
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
On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <[email protected]> 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 <[email protected]>
>---
>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.
On Wed, Apr 26, 2017 at 2:00 PM, <[email protected]> wrote:
> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <[email protected]> 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 <[email protected]>
>>---
>>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
On 04/26/2017 02:24 PM, [email protected] 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".
On April 26, 2017 2:21:25 PM PDT, Kees Cook <[email protected]> wrote:
>On Wed, Apr 26, 2017 at 2:00 PM, <[email protected]> wrote:
>> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke
><[email protected]> 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 <[email protected]>
>>>---
>>>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.
On Wed, Apr 26, 2017 at 1:55 PM, Matthias Kaehlcke <[email protected]> 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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
-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
On 04/26/17 14:29, Greg Hackmann wrote:
> On 04/26/2017 02:24 PM, [email protected] 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