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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2362 bytes --]

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

Its not as if the control flow would allow many different ways to reach
any of the hypfs_get_dyndata() calls.

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.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]