Discussion:
[PATCH] arm64: sysreg: Fix unprotected macro argmuent in write_sysreg
Dave Martin
2017-07-25 11:52:41 UTC
Permalink
write_sysreg() may misparse the value argument because it is used
without parentheses to protect it.

This patch adds the ( ) in order to avoid any surprises.

Signed-off-by: Dave Martin <***@arm.com>
---
arch/arm64/include/asm/sysreg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16e44fa..822eebc 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -492,7 +492,7 @@ asm(
* the "%x0" template means XZR.
*/
#define write_sysreg(v, r) do { \
- u64 __val = (u64)v; \
+ u64 __val = (u64)(v); \
asm volatile("msr " __stringify(r) ", %x0" \
: : "rZ" (__val)); \
} while (0)
--
2.1.4
Mark Rutland
2017-07-25 13:08:51 UTC
Permalink
Post by Dave Martin
write_sysreg() may misparse the value argument because it is used
without parentheses to protect it.
This patch adds the ( ) in order to avoid any surprises.
---
arch/arm64/include/asm/sysreg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16e44fa..822eebc 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -492,7 +492,7 @@ asm(
* the "%x0" template means XZR.
*/
#define write_sysreg(v, r) do { \
- u64 __val = (u64)v; \
+ u64 __val = (u64)(v); \
asm volatile("msr " __stringify(r) ", %x0" \
: : "rZ" (__val)); \
} while (0)
Looks sensible.

It looks like write_sysreg_s() will need the same fixup; could you do
that too?

With that:

Acked-by: Mark Rutland <***@arm.com>

Thanks,
Mark.
Dave Martin
2017-07-25 13:25:15 UTC
Permalink
Post by Mark Rutland
Post by Dave Martin
write_sysreg() may misparse the value argument because it is used
without parentheses to protect it.
This patch adds the ( ) in order to avoid any surprises.
---
arch/arm64/include/asm/sysreg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16e44fa..822eebc 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -492,7 +492,7 @@ asm(
* the "%x0" template means XZR.
*/
#define write_sysreg(v, r) do { \
- u64 __val = (u64)v; \
+ u64 __val = (u64)(v); \
asm volatile("msr " __stringify(r) ", %x0" \
: : "rZ" (__val)); \
} while (0)
Looks sensible.
It looks like write_sysreg_s() will need the same fixup; could you do
that too?
Agreed, especially if somebody else does it...

Note that there are some

write_sysreg(1 << n, SYS_FOO)

whose parse will be changed by this. However, n <= 30 in every case
I've found, which should mean there is no change to the result -- i.e.,
(u64)((int)1 << n) == (u64)1 << n if n <= 30.

Cheers
---Dave

Loading...