[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes
On 17.11.20 15:40, Jan Beulich wrote: On 17.11.2020 15:29, Jürgen Groß wrote:On 17.11.20 13:37, Jan Beulich wrote:On 26.10.2020 10:13, Juergen Gross wrote:--- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -19,28 +19,29 @@ CHECK_hypfs_dirlistentry; #endif-#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)-#define DIRENTRY_SIZE(name_len) \ - (DIRENTRY_NAME_OFF + \ - ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry))) - struct hypfs_funcs hypfs_dir_funcs = { .read = hypfs_read_dir, + .getsize = hypfs_getsize, + .findentry = hypfs_dir_findentry, }; struct hypfs_funcs hypfs_leaf_ro_funcs = { .read = hypfs_read_leaf, + .getsize = hypfs_getsize, }; struct hypfs_funcs hypfs_leaf_wr_funcs = { .read = hypfs_read_leaf, .write = hypfs_write_leaf, + .getsize = hypfs_getsize, }; struct hypfs_funcs hypfs_bool_wr_funcs = { .read = hypfs_read_leaf, .write = hypfs_write_bool, + .getsize = hypfs_getsize, }; struct hypfs_funcs hypfs_custom_wr_funcs = { .read = hypfs_read_leaf, .write = hypfs_write_custom, + .getsize = hypfs_getsize, };With the increasing number of fields that may (deliberately or by mistake) be NULL, should we gain some form of proactive guarding against calls through such pointers?Hmm, up to now I think such a bug would be detected rather fast.Not sure: Are there any unavoidable uses of all affected code paths? Assuming that any new node implementation is tested at least once, yes. But in general you are right, of course. I can add some ASSERT()s for mandatory functions not being NULL when a node is added dynamically or during hypfs initialization for the static nodes.I'm not sure ASSERT()s alone are enough. I'd definitely prefer something which at least avoids the obvious x86 PV privilege escalation attack in case a call through NULL has gone unnoticed earlier on. E.g. rather than storing NULL in unused entries, store a non-canonical pointer so that the effect will "just" be a DoS. Hmm, either this, or a dummy function doing no harm? @@ -88,6 +93,23 @@ static void hypfs_unlock(void) } }+void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)Will callers really need to specify (high) alignment values? IOW ...+{ + unsigned int cpu = smp_processor_id(); + + ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked); + ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL); + + per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);... is xzalloc_bytes() not suitable for use here?Good question. Up to now I think we could get away without specific alignment. I can drop that parameter for now if you'd like that better.I think control over alignment should be limited to those special cases really needing it. Okay, I'll drop the align parameter then. @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,l = container_of(entry, const struct hypfs_entry_leaf, e); - return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;+ return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ? + -EFAULT : 0;With the intended avoiding of locking, how is this ->getsize() guaranteed to not ...@@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry, goto out;ret = -ENOBUFS;- if ( ulen < entry->size + sizeof(e) ) + if ( ulen < size + sizeof(e) ) goto out;... invalidate the checking done here? (A similar risk looks to exist on the write path, albeit there we have at least the ->max_size checks, where I hope that field isn't mean to become dynamic as well.)I think you are right. I should add the size value as a parameter to the read and write functions.Except that a function like hypfs_read_leaf() shouldn't really require its caller to pass in the size of the leaf's payload. Its not the leaf's payload size, but the size the user buffer has been tested against. Juergen Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc Attachment:
OpenPGP_signature
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |