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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2020 13:41, 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.
> 
> Yes, but we do have tons of assumptions like that. I don't think we
> should add tests for non-NULL pointers everywhere just because we
> happen to dereference something. Where do we stop?
> 
>>
>>> 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.
> 
> No. The ASSERT() is clearly an attempt to catch a programming error
> early. It is especially not trying to catch a situation which is thought
> to be possible. The situation should really never happen, and I'm not
> aware how it could happen without a weird code modification.
> 
> Dropping the ASSERT() would really add risk to not notice a bug being
> introduced by a code modification.

Is this the case? Wouldn't the NULL be de-referenced almost immediately,
and hence the bug be noticed right away anyway? I don't think it is
typical for PV guests to have a valid mapping for address 0. Putting in
place such a mapping could at least be a hint towards possible malice
imo.

Jan