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: 3811 bytes --]

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.


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 --]