Re: [PATCH v3 5/8] xen/hypfs: add support for id-based dynamic directories

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2021 08:25, Jürgen Groß wrote:
> On 18.12.20 10:09, Jan Beulich wrote:
>> On 18.12.2020 09:57, Jürgen Groß wrote:
>>> On 17.12.20 13:14, Jan Beulich wrote:
>>>> On 17.12.2020 12:32, Jürgen Groß wrote:
>>>>> On 17.12.20 12:28, Jan Beulich wrote:
>>>>>> On 09.12.2020 17:09, Juergen Gross wrote:
>>>>>>> +static const struct hypfs_entry *hypfs_dyndir_enter(
>>>>>>> +    const struct hypfs_entry *entry)
>>>>>>> +{
>>>>>>> +    const struct hypfs_dyndir_id *data;
>>>>>>> +
>>>>>>> +    data = hypfs_get_dyndata();
>>>>>>> +
>>>>>>> +    /* Use template with original enter function. */
>>>>>>> +    return data->template->e.funcs->enter(&data->template->e);
>>>>>>> +}
>>>>>>
>>>>>> At the example of this (applies to other uses as well): I realize
>>>>>> hypfs_get_dyndata() asserts that the pointer is non-NULL, but
>>>>>> according to the bottom of ./CODING_STYLE this may not be enough
>>>>>> when considering the implications of a NULL deref in the context
>>>>>> of a PV guest. Even this living behind a sysctl doesn't really
>>>>>> help, both because via XSM not fully privileged domains can be
>>>>>> granted access, and because speculation may still occur all the
>>>>>> way into here. (I'll send a patch to address the latter aspect in
>>>>>> a few minutes.) While likely we have numerous existing examples
>>>>>> with similar problems, I guess in new code we'd better be as
>>>>>> defensive as possible.
>>>>>
>>>>> What do you suggest? BUG_ON()?
>>>>
>>>> Well, BUG_ON() would be a step in the right direction, converting
>>>> privilege escalation to DoS. The question is if we can't do better
>>>> here, gracefully failing in such a case (the usual pair of
>>>> ASSERT_UNREACHABLE() plus return/break/goto approach doesn't fit
>>>> here, at least not directly).
>>>>
>>>>> You are aware that this is nothing a user can influence, so it would
>>>>> be a clear coding error in the hypervisor?
>>>>
>>>> A user (or guest) can't arrange for there to be a NULL pointer,
>>>> but if there is one that can be run into here, this would still
>>>> require an XSA afaict.
>>>
>>> I still don't see how this could happen without a major coding bug,
>>> which IMO wouldn't go unnoticed during a really brief test (this is
>>> the reason for ASSERT() in hypfs_get_dyndata() after all).
>>
>> True. Yet the NULL derefs wouldn't go unnoticed either.
>>
>>> Its not as if the control flow would allow many different ways to reach
>>> any of the hypfs_get_dyndata() calls.
>>
>> I'm not convinced of this - this is a non-static function, and the
>> call patch 8 adds (just to take an example) is not very obvious to
>> have a guarantee that allocation did happen and was checked for
>> success. Yes, in principle cpupool_gran_write() isn't supposed to
>> be called in such a case, but it's the nature of bugs assumptions
>> get broken.
>>
>>> I can add security checks at the appropriate places, but I think this
>>> would be just dead code. OTOH if you are feeling strong here lets go
>>> with it.
>>
>> Going with it isn't the only possible route. The other is to drop
>> the ASSERT()s altogether. It simply seems to me that their addition
>> is a half-hearted attempt when considering what was added to
>> ./CODING_STYLE not all that long ago.
> 
> IMO The ASSERT()s are serving two purposes here: catching errors
> (which, as you stated already, might be catched later in any case),
> and documenting for the code reader that the condition they are
> testing should always be true and it a violation of it ought not to
> happen.
> 
> I can drop the ASSERT() calls, but I think they should be kept due
> to their documentation aspect.

Well, okay, I can see your point. Keep them in then, despite us
coming nowhere close to having similar NULL related asserts in
all similar places, I believe.

Jan