Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> 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.

>>> @@ -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.

>>> @@ -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.

Jan