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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.)

>> 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.

> 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.

~Andrew

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