Re: [Xen-devel] [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.17 at 22:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/04/17 09:19, Jan Beulich wrote:
>>>>> On 21.04.17 at 18:10, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 21/04/17 16:45, Jan Beulich wrote:
>>>>>>> On 21.04.17 at 16:05, <jennifer.herbert@xxxxxxxxxx> wrote:
>>>>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>>>>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>>>>> +
>>>>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>>>>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
>> (Side note: src also isn't properly parenthesized, and the title went
>> out of sync with the implementation.)
>>
>>>> Why all caps all of the sudden?
>>> This is the start of some code improvements, given the fallout from XSA-212.
>> I don't think making the names shout is an improvement in any way.
>> The #define-s above may still look fine, but the code using them is
>> now looking plain ugly (even more so with the yet longer names
>> introduced in patch 4).
> 
> That is a matter of opinion which I don't share, but ok.
> 
> As an alternative, how else do you suggest making it obvious to the
> reader of the code that this thing which looks like a function doesn't
> have function semantics?  (This is the purpose I am trying to get across.)

I do understand the intention, but I don't think capitalization of the
names helps this in any way. This is not the least because there are
compiler builtins which don't really have function semantics either
(e.g. __builtin_constant_p(), __builtin_expect(),
__builtin_types_compatible_p(), or __builtin_unreachable()). You
won't be able to do anything about these, other than perhaps
encapsulating them wrapper macros, but that still won't make the
compiler mandated name all caps.

Or take offsetof(), which following your argumentation then should
be - contrary to the C standard - OFFSETOF(), while what we have
as ASSERT() should then be - in line with the C standard, albeit our
macro has slightly different behavior - assert().

>>> make it more obvious to people reading the code that it *is not* a C
>>> function and doesn't behave like one.
>>>
>>> It is getting embarrassing how many security vulnerability we are seeing
>>> because macros look like they are doing one thing, yet actually do
>>> something else, and improving the quality of the code is the only way
>>> this is going to get better.
>> Considering the "how many" you use, mind giving three examples
>> where using all caps macro names would have made a difference?
>> I sincerely doubt that the case used in identifiers would make a
>> whole lot of a difference.
> 
> You have missed my point then.  We have many security vulnerabilities
> because we have deceptive code, and fix for that is to prevent the code
> being deceptive.  This is going to positive code quality effort on our
> behalf, because the status quo is currently terrible.
> 
> Most notably, XSA-212 just gone, where the root of the vulnerability is
> that "guest_handle_okay(base_ptr, array_element)" doesn't consider its
> second parameter, and degrades to checking just base_ptr.
> 
> I accept that, in this case, capitalising the macro wouldn't help, but
> that is because its deceptive nature is in its naming, not because it
> behaves in a way contrary to a C function.

A function could easily (and - by default - without warning) ignore
one or more of the arguments passed to it, too.

And as you say - XSA-212 is not an suitable example for your
argumentation here. Once again - I don't think name capitalization
helps in any way in what you want to ensure. And no, I don't think
I've missed your point here, I'm merely questioning the measure
you want to take to address your concerns. Therefore I'm still
lacking any examples of XSAs supporting your position regarding
identifier naming.

>> As a possible alternative, was it considered to pass pointers
>> here as before, using __builtin_object_size() on them instead of
>> the sizeof() above, and making the macros inline functions?
> 
> I have never tried using it in anger, but looking into it, it degrades
> to (size_t)-1 in the case the compiler can't statically work out the
> correct value.  As a result, you'd end up with a function which has
> gets() semantics (in the case that the compiler can't work out what's
> going on).  I don't recommend we use any constructs like this.

Well, I wouldn't rule it out without a little bit of experimenting.
While it can't be used in BUILD_BUG_ON(), it can very well be used
in ways similar to e.g. {read,write}_atomic(), adding a reference
to an always undefined symbol in the case the builtin produces 0
or -1.

On the grounds of wanting to not delay this series any further,
I'm about to commit it (albeit I note patch 2 is lacking a proper
maintainer ack, and I'm not going to add mine), but I'll make my
disagreement with the choice of macro names explicit, so these
names can't later be used as a reference to support naming
other macros in similarly ugly ways.

Jan

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