[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 Attachment:
OpenPGP_signature
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |