[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
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.