[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 5/8] xen/hypfs: add support for id-based dynamic directories
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.
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.
IMO The ASSERT()s are serving two purposes here: catching errors
(which, as you stated already, might be catched later in any case),
and documenting for the code reader that the condition they are
testing should always be true and it a violation of it ought not to
happen.
I can drop the ASSERT() calls, but I think they should be kept due
to their documentation aspect.
Replacing them with setting a negative return value is possible and
I will do it if you are insisting on it, but I still think this is
not a good solution, as it is adding code for a situation which
really should never happen (you could compare it to replacing
ASSERT(spin_is_locked()) cases with returning an error).
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
