[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.
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:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
