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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan