Re: [Xen-devel] Hazardous memset/memcpy idiom (was Re: [PATCH] x86: fix null pointer dereference in intel_get_extended_msrs())

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.02.13 at 10:25, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2013-02-27 at 08:50 +0000, Jan Beulich wrote:
>> >>> On 26.02.13 at 18:08, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 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


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