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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 

Jan