[PATCH for-4.14 0/3] Rework {read, write}_atomic()

[PATCH for-4.14 0/3] Rework {read, write}_atomic()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This small series is:
    - Hardening write_atomic() to prevent writing to const pointer
    - Allow {read, write}_atomic() to be used in more cases on Arm.

While this was posted after the last posting date, patch #1 is
necessary to avoid the cast introduced by Juergen in [1]. The rest of
the patches would be good hardening to have in Xen 4.14. So I would like
to request the full series to be included in Xen 4.14.

Cheers,

[1] <20200430152848.20275-1-jgross@suse.com>

CC: paul@xen.org

Julien Grall (3):
  xen/arm: atomic: Allow read_atomic() to be used in more cases
  xen/arm: atomic: Rewrite write_atomic()
  xen/x86: atomic: Don't allow to write atomically in a pointer to const

 xen/include/asm-arm/atomic.h | 77 ++++++++++++++++++++++++++----------
 xen/include/asm-x86/atomic.h |  2 +
 2 files changed, 59 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The current implementation of read_atomic() on Arm will not allow to:
    1) Read a value from a pointer to const because the temporary
    variable will be const and therefore it is not possible to assign
    any value. This can be solved by using a union between the type and
    a char[0].
    2) Read a pointer value (e.g void *) because the switch contains
    cast from other type than the size of a pointer. This can be solved by
    by introducing a static inline for the switch and use void * for the
    pointer.

Reported-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/atomic.h | 37 +++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index e81bf80e305c..3c3d6bb04ee8 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -71,18 +71,37 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t)
 #undef build_atomic_write
 #undef build_add_sized
 
+void __bad_atomic_read(const volatile void *p, void *res);
 void __bad_atomic_size(void);
 
+static always_inline void read_atomic_size(const volatile void *p,
+                                           void *res,
+                                           unsigned int size)
+{
+    switch ( size )
+    {
+    case 1:
+        *(uint8_t *)res = read_u8_atomic(p);
+        break;
+    case 2:
+        *(uint16_t *)res = read_u16_atomic(p);
+        break;
+    case 4:
+        *(uint32_t *)res = read_u32_atomic(p);
+        break;
+    case 8:
+        *(uint64_t *)res = read_u64_atomic(p);
+        break;
+    default:
+        __bad_atomic_read(p, res);
+        break;
+    }
+}
+
 #define read_atomic(p) ({                                               \
-    typeof(*p) __x;                                                     \
-    switch ( sizeof(*p) ) {                                             \
-    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
-    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
-    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
-    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
-    default: __x = 0; __bad_atomic_size(); break;                       \
-    }                                                                   \
-    __x;                                                                \
+    union { typeof(*p) val; char c[0]; } x_;                            \
+    read_atomic_size(p, x_.c, sizeof(*p));                              \
+    x_.val;                                                             \
 })
 
 #define write_atomic(p, x) ({                                           \
-- 
2.17.1

[PATCH for-4.14 2/3] xen/arm: atomic: Rewrite write_atomic()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The current implementation of write_atomic has two issues:
    1) It cannot be used to write pointer value because the switch
    contains cast to other size than the size of the pointer.
    2) It will happily allow to write to a pointer to const.

Additionally, the Arm implementation is returning a value when the x86
implementation does not anymore. This was introduced in commit
2934148a0773 "x86: simplify a few macros / inline functions". There are
no users of the return value, so it is fine to drop it.

The switch is now moved in a static inline helper allowing the compiler
to prevent use of const pointer and also allow to write pointer value.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/atomic.h | 40 ++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index 3c3d6bb04ee8..ac2798d095eb 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -98,23 +98,41 @@ static always_inline void read_atomic_size(const volatile void *p,
     }
 }
 
+static always_inline void write_atomic_size(volatile void *p,
+                                            void *val,
+                                            unsigned int size)
+{
+    switch ( size )
+    {
+    case 1:
+        write_u8_atomic(p, *(uint8_t *)val);
+        break;
+    case 2:
+        write_u16_atomic(p, *(uint16_t *)val);
+        break;
+    case 4:
+        write_u32_atomic(p, *(uint32_t *)val);
+        break;
+    case 8:
+        write_u64_atomic(p, *(uint64_t *)val);
+        break;
+    default:
+        __bad_atomic_size();
+        break;
+    }
+}
+
 #define read_atomic(p) ({                                               \
     union { typeof(*p) val; char c[0]; } x_;                            \
     read_atomic_size(p, x_.c, sizeof(*p));                              \
     x_.val;                                                             \
 })
 
-#define write_atomic(p, x) ({                                           \
-    typeof(*p) __x = (x);                                               \
-    switch ( sizeof(*p) ) {                                             \
-    case 1: write_u8_atomic((uint8_t *)p, (uint8_t)__x); break;         \
-    case 2: write_u16_atomic((uint16_t *)p, (uint16_t)__x); break;      \
-    case 4: write_u32_atomic((uint32_t *)p, (uint32_t)__x); break;      \
-    case 8: write_u64_atomic((uint64_t *)p, (uint64_t)__x); break;      \
-    default: __bad_atomic_size(); break;                                \
-    }                                                                   \
-    __x;                                                                \
-})
+#define write_atomic(p, x)                                              \
+    do {                                                                \
+        typeof(*p) x_ = (x);                                            \
+        write_atomic_size(p, &x_, sizeof(*p));                          \
+    } while ( false )
 
 #define add_sized(p, x) ({                                              \
     typeof(*(p)) __x = (x);                                             \
-- 
2.17.1

[PATCH for-4.14 3/3] xen/x86: atomic: Don't allow to write atomically in a pointer to const

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, write_atomic() will happily write to a pointer to const.
While there are no use in Xen, it would be best to catch them at
compilation time.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-x86/atomic.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index 6b40f9c9f872..0a332b1fae18 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -63,6 +63,8 @@ void __bad_atomic_size(void);
 
 #define write_atomic(p, x) ({                             \
     typeof(*(p)) __x = (x);                               \
+    /* Check that the pointer is not const */             \
+    void *__maybe_unused p_ = &__x;                       \
     unsigned long x_ = (unsigned long)__x;                \
     switch ( sizeof(*(p)) ) {                             \
     case 1: write_u8_atomic((uint8_t *)(p), x_); break;   \
-- 
2.17.1

Re: [PATCH for-4.14 3/3] xen/x86: atomic: Don't allow to write atomically in a pointer to const

[Jan Beulich]

On 02.05.2020 18:07, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, write_atomic() will happily write to a pointer to const.
> While there are no use in Xen, it would be best to catch them at
> compilation time.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- a/xen/include/asm-x86/atomic.h
> +++ b/xen/include/asm-x86/atomic.h
> @@ -63,6 +63,8 @@ void __bad_atomic_size(void);
>  
>  #define write_atomic(p, x) ({                             \
>      typeof(*(p)) __x = (x);                               \
> +    /* Check that the pointer is not const */             \
> +    void *__maybe_unused p_ = &__x;                       \

... along the lines of the similar case with guest handles I'd
like to suggest for the comment to be more precise: It's not
the pointer's const-ness you're after, but the pointed to
object's. Maybe "Check that the pointer is not to a const
type" or even just "Check that the pointer is not to const"?

Jan

Re: [PATCH for-4.14 3/3] xen/x86: atomic: Don't allow to write atomically in a pointer to const

[Julien Grall]

Hi Jan,

On 04/05/2020 11:47, Jan Beulich wrote:
> On 02.05.2020 18:07, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, write_atomic() will happily write to a pointer to const.
>> While there are no use in Xen, it would be best to catch them at
>> compilation time.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> albeit ...
> 
>> --- a/xen/include/asm-x86/atomic.h
>> +++ b/xen/include/asm-x86/atomic.h
>> @@ -63,6 +63,8 @@ void __bad_atomic_size(void);
>>   
>>   #define write_atomic(p, x) ({                             \
>>       typeof(*(p)) __x = (x);                               \
>> +    /* Check that the pointer is not const */             \
>> +    void *__maybe_unused p_ = &__x;                       \
> 
> ... along the lines of the similar case with guest handles I'd
> like to suggest for the comment to be more precise: It's not
> the pointer's const-ness you're after, but the pointed to
> object's. Maybe "Check that the pointer is not to a const
> type" or even just "Check that the pointer is not to const"?

I am happy with "Check that the pointer is not to a const type".

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Stefano Stabellini]

On Sat, 2 May 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The current implementation of read_atomic() on Arm will not allow to:
>     1) Read a value from a pointer to const because the temporary
>     variable will be const and therefore it is not possible to assign
>     any value. This can be solved by using a union between the type and
>     a char[0].
>     2) Read a pointer value (e.g void *) because the switch contains
>     cast from other type than the size of a pointer. This can be solved by
>     by introducing a static inline for the switch and use void * for the
>     pointer.
> 
> Reported-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/include/asm-arm/atomic.h | 37 +++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
> index e81bf80e305c..3c3d6bb04ee8 100644
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -71,18 +71,37 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t)
>  #undef build_atomic_write
>  #undef build_add_sized
>  
> +void __bad_atomic_read(const volatile void *p, void *res);
>  void __bad_atomic_size(void);
>  
> +static always_inline void read_atomic_size(const volatile void *p,
> +                                           void *res,
> +                                           unsigned int size)
> +{
> +    switch ( size )
> +    {
> +    case 1:
> +        *(uint8_t *)res = read_u8_atomic(p);
> +        break;
> +    case 2:
> +        *(uint16_t *)res = read_u16_atomic(p);
> +        break;
> +    case 4:
> +        *(uint32_t *)res = read_u32_atomic(p);
> +        break;
> +    case 8:
> +        *(uint64_t *)res = read_u64_atomic(p);
> +        break;
> +    default:
> +        __bad_atomic_read(p, res);
> +        break;
> +    }
> +}
> +
>  #define read_atomic(p) ({                                               \
> -    typeof(*p) __x;                                                     \
> -    switch ( sizeof(*p) ) {                                             \
> -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
> -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
> -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
> -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
> -    default: __x = 0; __bad_atomic_size(); break;                       \
> -    }                                                                   \
> -    __x;                                                                \
> +    union { typeof(*p) val; char c[0]; } x_;                            \
> +    read_atomic_size(p, x_.c, sizeof(*p));                              \

Wouldn't it be better to pass x_ as follows:

    read_atomic_size(p, &x_, sizeof(*p));

?

In my tests both ways works. I prefer the version with &x_ but given
that it works either way:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>



> +    x_.val;                                                             \
>  })
>  
>  #define write_atomic(p, x) ({                                           \
> -- 
> 2.17.1
> 

Re: [PATCH for-4.14 2/3] xen/arm: atomic: Rewrite write_atomic()

[Stefano Stabellini]

On Sat, 2 May 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The current implementation of write_atomic has two issues:
>     1) It cannot be used to write pointer value because the switch
>     contains cast to other size than the size of the pointer.
>     2) It will happily allow to write to a pointer to const.
> 
> Additionally, the Arm implementation is returning a value when the x86
> implementation does not anymore. This was introduced in commit
> 2934148a0773 "x86: simplify a few macros / inline functions". There are
> no users of the return value, so it is fine to drop it.
> 
> The switch is now moved in a static inline helper allowing the compiler
> to prevent use of const pointer and also allow to write pointer value.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/include/asm-arm/atomic.h | 40 ++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
> index 3c3d6bb04ee8..ac2798d095eb 100644
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -98,23 +98,41 @@ static always_inline void read_atomic_size(const volatile void *p,
>      }
>  }
>  
> +static always_inline void write_atomic_size(volatile void *p,
> +                                            void *val,
> +                                            unsigned int size)
> +{
> +    switch ( size )
> +    {
> +    case 1:
> +        write_u8_atomic(p, *(uint8_t *)val);
> +        break;
> +    case 2:
> +        write_u16_atomic(p, *(uint16_t *)val);
> +        break;
> +    case 4:
> +        write_u32_atomic(p, *(uint32_t *)val);
> +        break;
> +    case 8:
> +        write_u64_atomic(p, *(uint64_t *)val);
> +        break;
> +    default:
> +        __bad_atomic_size();
> +        break;
> +    }
> +}
> +
>  #define read_atomic(p) ({                                               \
>      union { typeof(*p) val; char c[0]; } x_;                            \
>      read_atomic_size(p, x_.c, sizeof(*p));                              \
>      x_.val;                                                             \
>  })
>  
> -#define write_atomic(p, x) ({                                           \
> -    typeof(*p) __x = (x);                                               \
> -    switch ( sizeof(*p) ) {                                             \
> -    case 1: write_u8_atomic((uint8_t *)p, (uint8_t)__x); break;         \
> -    case 2: write_u16_atomic((uint16_t *)p, (uint16_t)__x); break;      \
> -    case 4: write_u32_atomic((uint32_t *)p, (uint32_t)__x); break;      \
> -    case 8: write_u64_atomic((uint64_t *)p, (uint64_t)__x); break;      \
> -    default: __bad_atomic_size(); break;                                \
> -    }                                                                   \
> -    __x;                                                                \
> -})
> +#define write_atomic(p, x)                                              \
> +    do {                                                                \
> +        typeof(*p) x_ = (x);                                            \
> +        write_atomic_size(p, &x_, sizeof(*p));                          \
> +    } while ( false )
>  
>  #define add_sized(p, x) ({                                              \
>      typeof(*(p)) __x = (x);                                             \
> -- 
> 2.17.1
> 

Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Julien Grall]

Hi,

On 07/05/2020 21:29, Stefano Stabellini wrote:
>>   #define read_atomic(p) ({                                               \
>> -    typeof(*p) __x;                                                     \
>> -    switch ( sizeof(*p) ) {                                             \
>> -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
>> -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
>> -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
>> -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
>> -    default: __x = 0; __bad_atomic_size(); break;                       \
>> -    }                                                                   \
>> -    __x;                                                                \
>> +    union { typeof(*p) val; char c[0]; } x_;                            \
>> +    read_atomic_size(p, x_.c, sizeof(*p));                              \
> 
> Wouldn't it be better to pass x_ as follows:
> 
>      read_atomic_size(p, &x_, sizeof(*p));

I am not sure to understand this. Are you suggesting to pass a pointer 
to the union?

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Stefano Stabellini]

On Thu, 7 May 2020, Julien Grall wrote:
> Hi,
> 
> On 07/05/2020 21:29, Stefano Stabellini wrote:
> > >   #define read_atomic(p) ({
> > > \
> > > -    typeof(*p) __x;                                                     \
> > > -    switch ( sizeof(*p) ) {                                             \
> > > -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
> > > -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
> > > -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
> > > -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
> > > -    default: __x = 0; __bad_atomic_size(); break;                       \
> > > -    }                                                                   \
> > > -    __x;                                                                \
> > > +    union { typeof(*p) val; char c[0]; } x_;                            \
> > > +    read_atomic_size(p, x_.c, sizeof(*p));                              \
> > 
> > Wouldn't it be better to pass x_ as follows:
> > 
> >      read_atomic_size(p, &x_, sizeof(*p));
> 
> I am not sure to understand this. Are you suggesting to pass a pointer to the
> union?

Yes. Would it cause a problem that I couldn't spot?

Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Julien Grall]

Hi Stefano,

On 07/05/2020 21:34, Stefano Stabellini wrote:
> On Thu, 7 May 2020, Julien Grall wrote:
>> Hi,
>>
>> On 07/05/2020 21:29, Stefano Stabellini wrote:
>>>>    #define read_atomic(p) ({
>>>> \
>>>> -    typeof(*p) __x;                                                     \
>>>> -    switch ( sizeof(*p) ) {                                             \
>>>> -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
>>>> -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
>>>> -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
>>>> -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
>>>> -    default: __x = 0; __bad_atomic_size(); break;                       \
>>>> -    }                                                                   \
>>>> -    __x;                                                                \
>>>> +    union { typeof(*p) val; char c[0]; } x_;                            \
>>>> +    read_atomic_size(p, x_.c, sizeof(*p));                              \
>>>
>>> Wouldn't it be better to pass x_ as follows:
>>>
>>>       read_atomic_size(p, &x_, sizeof(*p));
>>
>> I am not sure to understand this. Are you suggesting to pass a pointer to the
>> union?
> 
> Yes. Would it cause a problem that I couldn't spot?

You defeat the purpose of an union by casting it to something else (even 
if it is void *).

The goal of the union is to be able to access a value in different way 
through a member. So x_.c is more union friendly and makes easier to 
understand why it was implemented like this.

Cheers,

-- 
Julien Grall