Xen devel - Feb 2013 - [PATCH] x86: fix null pointer dereference in intel_get_extended_msrs()

`memset(&mc_ext, 0, ...)'' leads to a buffer overflow and a
subsequent
null pointer dereference.  Replace `&mc_ext'' with
`mc_ext''.

Signed-off-by: Xi Wang <xi@mit.edu>
---
 xen/arch/x86/cpu/mcheck/mce_intel.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c
b/xen/arch/x86/cpu/mcheck/mce_intel.c
index d80f692..57e93d4 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -200,7 +200,7 @@ intel_get_extended_msrs(struct mcinfo_global *mig, struct
mc_info *mi)
     }
 
     /* this function will called when CAP(9).MCG_EXT_P = 1 */
-    memset(&mc_ext, 0, sizeof(struct mcinfo_extended));
+    memset(mc_ext, 0, sizeof(struct mcinfo_extended));
     mc_ext->common.type = MC_TYPE_EXTENDED;
     mc_ext->common.size = sizeof(struct mcinfo_extended);
 
-- 
1.7.10.4

Xi Wang writes ("[Xen-devel] [PATCH] x86: fix null pointer dereference in
intel_get_extended_msrs()"):
> `memset(&mc_ext, 0, ...)'' leads to a buffer overflow and a
subsequent
> null pointer dereference.  Replace `&mc_ext'' with
`mc_ext''.
Really I think we shouldn''t be writing out these kind of memsets.
They''re too error-prone.  We should have a macro, perhaps like this:

  #define FILLZERO(object) memset(&(object), 0, sizeof(object))

Likewise a copy macro.

Ian.

>>> On 26.02.13 at 18:08, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Xi Wang writes ("[Xen-devel] [PATCH] x86: fix null pointer dereference
in
> intel_get_extended_msrs()"):
>> `memset(&mc_ext, 0, ...)'' leads to a buffer overflow and a
subsequent
>> null pointer dereference.  Replace `&mc_ext'' with
`mc_ext''.
> 
> Really I think we shouldn''t be writing out these kind of memsets.
> They''re too error-prone.  We should have a macro, perhaps like
this:
> 
>   #define FILLZERO(object) memset(&(object), 0, sizeof(object))
> 
> Likewise a copy macro.
Unfortunately that wouldn''t be usable in all cases (arrays passed
to functions), and hence I''m not really confident this would be a
good move. But yes, I considered what you suggest a number of
times already, but dropped the idea each time because of its lack
of general applicability.

I''m afraid we just have to live with the bad side effects of the
power C provides.

Jan

On Wed, 2013-02-27 at 08:50 +0000, Jan Beulich wrote:
> >>> On 26.02.13 at 18:08, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> > Xi Wang writes ("[Xen-devel] [PATCH] x86: fix null pointer
dereference in
> > intel_get_extended_msrs()"):
> >> `memset(&mc_ext, 0, ...)'' leads to a buffer overflow
and a subsequent
> >> null pointer dereference.  Replace `&mc_ext'' with
`mc_ext''.
> > 
> > Really I think we shouldn''t be writing out these kind of
memsets.
> > They''re too error-prone.  We should have a macro, perhaps
like this:
> > 
> >   #define FILLZERO(object) memset(&(object), 0, sizeof(object))
> > 
> > Likewise a copy macro.
> 
> Unfortunately that wouldn''t be usable in all cases (arrays passed
> to functions), and hence I''m not really confident this would be a
> good move. But yes, I considered what you suggest a number of
> times already, but dropped the idea each time because of its lack
> of general applicability.
> 
> I''m afraid we just have to live with the bad side effects of the
> power C provides.
In this specific case perhaps x86_mcinfo_reserve() should zero the
buffer instead of each caller having to do the right thing? Most callers
have a memset, a couple immediately fill in the buffer instead of
zeroing it, but this isn''t a hot path, is it?

Ian.

>>> On 27.02.13 at 10:25, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Wed, 2013-02-27 at 08:50 +0000, Jan Beulich wrote:
>> >>> On 26.02.13 at 18:08, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
>> > Xi Wang writes ("[Xen-devel] [PATCH] x86: fix null pointer
dereference in
>> > intel_get_extended_msrs()"):
>> >> `memset(&mc_ext, 0, ...)'' leads to a buffer
overflow and a subsequent
>> >> null pointer dereference.  Replace `&mc_ext'' with
`mc_ext''.
>> > 
>> > Really I think we shouldn''t be writing out these kind of
memsets.
>> > They''re too error-prone.  We should have a macro, perhaps
like this:
>> > 
>> >   #define FILLZERO(object) memset(&(object), 0,
sizeof(object))
>> > 
>> > Likewise a copy macro.
>> 
>> Unfortunately that wouldn''t be usable in all cases (arrays
passed
>> to functions), and hence I''m not really confident this would
be a
>> good move. But yes, I considered what you suggest a number of
>> times already, but dropped the idea each time because of its lack
>> of general applicability.
>> 
>> I''m afraid we just have to live with the bad side effects of
the
>> power C provides.
> 
> In this specific case perhaps x86_mcinfo_reserve() should zero the
> buffer instead of each caller having to do the right thing? Most callers
> have a memset, a couple immediately fill in the buffer instead of
> zeroing it, but this isn''t a hot path, is it?
Yes, I think we should. I''ll prepare a patch. Nevertheless, the
original patch is desirable to have as a separate one, as that''s
what I would prefer for backporting.

Jan

Jan Beulich writes ("Hazardous memset/memcpy idiom (was Re: [Xen-devel]
[PATCH] x86: fix null pointer dereference in
intel_get_extended_msrs())"):
> Unfortunately that wouldn''t be usable in all cases (arrays passed
> to functions),
 #define FILLZERO_ARRAY(arrayobj, num_elements) \
    (memset((arrayobj),0,sizeof((arrayobj)[0])*(num_elements)))

And the fact that a macro isn''t universally applicable isn''t a
reason
not to replace an error-prone idiom in the very frequent cases where
the macro would be applicable.

Ian.