Re: [PATCH v3 5/8] xen/hypfs: add support for id-based dynamic directories

[Jürgen Groß <jgross@xxxxxxxx>]

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()?

You are aware that this is nothing a user can influence, so it would
be a clear coding error in the hypervisor?

> > +/*
> > + * Fill dyndata with a dynamically generated directory based on a template
> > + * and a numerical id.
> > + * Needs to be kept in sync with hypfs_read_dyndir_id_entry() regarding the
> > + * name generated.
> > + */
> > +struct hypfs_entry *hypfs_gen_dyndir_id_entry(
> > +    const struct hypfs_entry_dir *template, unsigned int id, void *data)
> > +{
> s/directory/entry/ in the comment (and as a I realize only now
> then also for hypfs_read_dyndir_id_entry())?
Oh, indeed.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature