[PATCH 1/2] x86/mm: Avoid NULL dereference when checking altp2m's for shareability

[PATCH 1/2] x86/mm: Avoid NULL dereference when checking altp2m's for shareability

[Andrew Cooper]

Coverity identifies that __get_gfn_type_access() unconditionally writes to its
type parameter under a number of circumstances.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>

There is a second complaint that ap2ma and p2ma are used before initialisation
in the following line, although that is harder to reason about.  I think the
code is OK...
---
 xen/arch/x86/mm/mem_sharing.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 47e0820..14952ce 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -870,6 +870,7 @@ int mem_sharing_nominate_page(struct domain *d,
         unsigned int i;
         struct p2m_domain *ap2m;
         mfn_t amfn;
+        p2m_type_t ap2mt;
         p2m_access_t ap2ma;
 
         altp2m_list_lock(d);
@@ -880,7 +881,7 @@ int mem_sharing_nominate_page(struct domain *d,
             if ( !ap2m )
                 continue;
 
-            amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
+            amfn = get_gfn_type_access(ap2m, gfn, &ap2mt, &ap2ma, 0, NULL);
             if ( mfn_valid(amfn) && (mfn_x(amfn) != mfn_x(mfn) || ap2ma != p2ma) )
             {
                 altp2m_list_unlock(d);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Andrew Cooper]

Introduce and use the nonnull attribute to help the compiler catch NULL
parameters being passed to function which require their parameters not to be
NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
from immediate callers, so propagate the attributes out to all helpers.

A sample error looks like:

mem_sharing.c: In function ‘mem_sharing_nominate_page’:
mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
             amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
             ^

As part of this, replace the get_gfn_type_access() macro with an equivalent
static inline function for extra type safety, and the ability to be annotated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 xen/include/asm-x86/p2m.h  | 19 +++++++++++--------
 xen/include/xen/compiler.h |  2 ++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 194020e..e35d59c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -380,9 +380,9 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m);
  * After calling any of the variants below, caller needs to use
  * put_gfn. ****/
 
-mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
-                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
-                    unsigned int *page_order, bool_t locked);
+mfn_t __nonnull(1, 3, 4) __get_gfn_type_access(
+    struct p2m_domain *p2m, unsigned long gfn, p2m_type_t *t,
+    p2m_access_t *a, p2m_query_t q, unsigned int *page_order, bool_t locked);
 
 /* Read a particular P2M table, mapping pages as we go.  Most callers
  * should _not_ call this directly; use the other get_gfn* functions
@@ -391,13 +391,16 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
  * If the lookup succeeds, the return value is != INVALID_MFN and 
  * *page_order is filled in with the order of the superpage (if any) that
  * the entry was found in.  */
-#define get_gfn_type_access(p, g, t, a, q, o)   \
-        __get_gfn_type_access((p), (g), (t), (a), (q), (o), 1)
+static inline mfn_t __nonnull(1, 3, 4) get_gfn_type_access(
+    struct p2m_domain *p2m, unsigned long gfn, p2m_type_t *t,
+    p2m_access_t *a, p2m_query_t q, unsigned int *page_order)
+{
+    return __get_gfn_type_access(p2m, gfn, t, a, q, page_order, true);
+}
 
 /* General conversion function from gfn to mfn */
-static inline mfn_t get_gfn_type(struct domain *d,
-                                    unsigned long gfn, p2m_type_t *t,
-                                    p2m_query_t q)
+static inline mfn_t __nonnull(1, 3) get_gfn_type(
+    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
     p2m_access_t a;
     return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 892455b..8fcb033 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -62,6 +62,8 @@
 
 #define __must_check __attribute__((__warn_unused_result__))
 
+#define __nonnull(...) __attribute__((nonnull (__VA_ARGS__)))
+
 #define offsetof(a,b) __builtin_offsetof(a,b)
 
 #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[George Dunlap]

On 27/07/16 19:08, Andrew Cooper wrote:
> Introduce and use the nonnull attribute to help the compiler catch NULL
> parameters being passed to function which require their parameters not to be
> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
> from immediate callers, so propagate the attributes out to all helpers.
> 
> A sample error looks like:
> 
> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
> mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>              ^
> 
> As part of this, replace the get_gfn_type_access() macro with an equivalent
> static inline function for extra type safety, and the ability to be annotated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

At a high level this looks like it's probably an improvement; I'd like
to hear opinions of people who tend to have stronger opinions here first.

One technical comment...

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
>  xen/include/asm-x86/p2m.h  | 19 +++++++++++--------
>  xen/include/xen/compiler.h |  2 ++
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 194020e..e35d59c 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -380,9 +380,9 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m);
>   * After calling any of the variants below, caller needs to use
>   * put_gfn. ****/
>  
> -mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
> -                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
> -                    unsigned int *page_order, bool_t locked);
> +mfn_t __nonnull(1, 3, 4) __get_gfn_type_access(

__get_gfn_type_access() explicitly tolerates p2m being NULL, so '1'
should be removed from the list (both here and below).

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Andrew Cooper]

On 28/07/16 16:58, George Dunlap wrote:
> On 27/07/16 19:08, Andrew Cooper wrote:
>> Introduce and use the nonnull attribute to help the compiler catch NULL
>> parameters being passed to function which require their parameters not to be
>> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
>> from immediate callers, so propagate the attributes out to all helpers.
>>
>> A sample error looks like:
>>
>> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
>> mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
>>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>>              ^
>>
>> As part of this, replace the get_gfn_type_access() macro with an equivalent
>> static inline function for extra type safety, and the ability to be annotated.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> At a high level this looks like it's probably an improvement; I'd like
> to hear opinions of people who tend to have stronger opinions here first.
>
> One technical comment...
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>>  xen/include/asm-x86/p2m.h  | 19 +++++++++++--------
>>  xen/include/xen/compiler.h |  2 ++
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 194020e..e35d59c 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -380,9 +380,9 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m);
>>   * After calling any of the variants below, caller needs to use
>>   * put_gfn. ****/
>>  
>> -mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
>> -                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
>> -                    unsigned int *page_order, bool_t locked);
>> +mfn_t __nonnull(1, 3, 4) __get_gfn_type_access(
> __get_gfn_type_access() explicitly tolerates p2m being NULL, so '1'
> should be removed from the list (both here and below).

So it does.  I wonder why that is?  I presume PV guests don't have a p2m.

Looking though this code, it seems to be an unnecessarily complicated
tangle :s

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Jan Beulich]

>>> On 28.07.16 at 17:58, <george.dunlap@citrix.com> wrote:
> On 27/07/16 19:08, Andrew Cooper wrote:
>> Introduce and use the nonnull attribute to help the compiler catch NULL
>> parameters being passed to function which require their parameters not to be
>> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
>> from immediate callers, so propagate the attributes out to all helpers.
>> 
>> A sample error looks like:
>> 
>> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
>> mem_sharing.c:884:13: error: null argument where non-null required (argument 
> 3) [-Werror=nonnull]
>>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>>              ^
>> 
>> As part of this, replace the get_gfn_type_access() macro with an equivalent
>> static inline function for extra type safety, and the ability to be 
> annotated.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> At a high level this looks like it's probably an improvement; I'd like
> to hear opinions of people who tend to have stronger opinions here first.

I agree on this being a desirable change.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Andrew Cooper]

Introduce and use the nonnull attribute to help the compiler catch NULL
parameters being passed to function which require their parameters not to be
NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
from immediate callers, so propagate the attributes out to all helpers.

A sample error looks like:

mem_sharing.c: In function ‘mem_sharing_nominate_page’:
mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
             amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
             ^

As part of this, replace the get_gfn_type_access() macro with an equivalent
static inline function for extra type safety, and the ability to be annotated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>

v2:
 * s/nonnull/__nonnull__/
 * Tolerate the p2m parameter being NULL
---
 xen/include/asm-x86/p2m.h  | 19 +++++++++++--------
 xen/include/xen/compiler.h |  1 +
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 194020e..035ca92 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -380,9 +380,9 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m);
  * After calling any of the variants below, caller needs to use
  * put_gfn. ****/
 
-mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
-                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
-                    unsigned int *page_order, bool_t locked);
+mfn_t __nonnull(3, 4) __get_gfn_type_access(
+    struct p2m_domain *p2m, unsigned long gfn, p2m_type_t *t,
+    p2m_access_t *a, p2m_query_t q, unsigned int *page_order, bool_t locked);
 
 /* Read a particular P2M table, mapping pages as we go.  Most callers
  * should _not_ call this directly; use the other get_gfn* functions
@@ -391,13 +391,16 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
  * If the lookup succeeds, the return value is != INVALID_MFN and 
  * *page_order is filled in with the order of the superpage (if any) that
  * the entry was found in.  */
-#define get_gfn_type_access(p, g, t, a, q, o)   \
-        __get_gfn_type_access((p), (g), (t), (a), (q), (o), 1)
+static inline mfn_t __nonnull(3, 4) get_gfn_type_access(
+    struct p2m_domain *p2m, unsigned long gfn, p2m_type_t *t,
+    p2m_access_t *a, p2m_query_t q, unsigned int *page_order)
+{
+    return __get_gfn_type_access(p2m, gfn, t, a, q, page_order, true);
+}
 
 /* General conversion function from gfn to mfn */
-static inline mfn_t get_gfn_type(struct domain *d,
-                                    unsigned long gfn, p2m_type_t *t,
-                                    p2m_query_t q)
+static inline mfn_t __nonnull(3) get_gfn_type(
+    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
     p2m_access_t a;
     return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 892455b..f3e8d95 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -61,6 +61,7 @@
 #define __maybe_unused __attribute__((__unused__))
 
 #define __must_check __attribute__((__warn_unused_result__))
+#define __nonnull(...) __attribute__((__nonnull__(__VA_ARGS__)))
 
 #define offsetof(a,b) __builtin_offsetof(a,b)
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Jan Beulich]

>>> On 01.08.16 at 18:59, <andrew.cooper3@citrix.com> wrote:
> Introduce and use the nonnull attribute to help the compiler catch NULL
> parameters being passed to function which require their parameters not to be
> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
> from immediate callers, so propagate the attributes out to all helpers.
> 
> A sample error looks like:
> 
> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
> mem_sharing.c:884:13: error: null argument where non-null required (argument 
> 3) [-Werror=nonnull]
>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>              ^
> 
> As part of this, replace the get_gfn_type_access() macro with an equivalent
> static inline function for extra type safety, and the ability to be 
> annotated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[George Dunlap]

On Mon, Aug 1, 2016 at 5:59 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Introduce and use the nonnull attribute to help the compiler catch NULL
> parameters being passed to function which require their parameters not to be
> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
> from immediate callers, so propagate the attributes out to all helpers.
>
> A sample error looks like:
>
> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
> mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>              ^
>
> As part of this, replace the get_gfn_type_access() macro with an equivalent
> static inline function for extra type safety, and the ability to be annotated.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Jan Beulich]

>>> On 27.07.16 at 20:08, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -62,6 +62,8 @@
>  
>  #define __must_check __attribute__((__warn_unused_result__))
>  
> +#define __nonnull(...) __attribute__((nonnull (__VA_ARGS__)))

May I ask to use __nonnull__ here, to match the #define right
above? Also I don't think there should be a blank following that
token (again matching other #define-s further up).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86/mm: Avoid NULL dereference when checking altp2m's for shareability

[George Dunlap]

On 27/07/16 19:08, Andrew Cooper wrote:
> Coverity identifies that __get_gfn_type_access() unconditionally writes to its
> type parameter under a number of circumstances.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> 
> There is a second complaint that ap2ma and p2ma are used before initialisation
> in the following line, although that is harder to reason about.  I think the
> code is OK...

Well there are paths through __get_gfn_type_access() which don't set the
access value -- namely if p2m is null or
!paging_mode_translate(p2m->domain) (which coverity has no way of knowing).

That probably could use being made more robust at some point.

 -George

> ---
>  xen/arch/x86/mm/mem_sharing.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 47e0820..14952ce 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -870,6 +870,7 @@ int mem_sharing_nominate_page(struct domain *d,
>          unsigned int i;
>          struct p2m_domain *ap2m;
>          mfn_t amfn;
> +        p2m_type_t ap2mt;
>          p2m_access_t ap2ma;
>  
>          altp2m_list_lock(d);
> @@ -880,7 +881,7 @@ int mem_sharing_nominate_page(struct domain *d,
>              if ( !ap2m )
>                  continue;
>  
> -            amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
> +            amfn = get_gfn_type_access(ap2m, gfn, &ap2mt, &ap2ma, 0, NULL);
>              if ( mfn_valid(amfn) && (mfn_x(amfn) != mfn_x(mfn) || ap2ma != p2ma) )
>              {
>                  altp2m_list_unlock(d);
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel