Re: [Minios-devel] [UNIKRAFT PATCH] lib/vfscore: fix null pointer dereferences

[Hugo Lefeuvre <hle@xxxxxxxxxx>]

Hi Simon,

> thanks a lot for this work, I have some comments inline.

thanks for your review! answers inline.

Please jump right to the v3, I found another issue in my patch meanwhile :-)

> On 28.02.20 14:22, Hugo Lefeuvre wrote:
> > dentry_alloc and dentry_move both create dentry d_path fields using
> > strdup, without checking for NULL return values. This leads to null
> > pointer dereferences if the allocator goes OOM.
> > 
> > Modify dentry_move to return an error code (0 for success, otherwise
> > error code).
> > 
> > Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> > ---
> >   lib/vfscore/dentry.c                 | 12 +++++++++++-
> >   lib/vfscore/include/vfscore/dentry.h |  2 +-
> >   lib/vfscore/syscalls.c               |  5 ++++-
> >   3 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/vfscore/dentry.c b/lib/vfscore/dentry.c
> > index 76f7a6b..a4728fc 100644
> > --- a/lib/vfscore/dentry.c
> > +++ b/lib/vfscore/dentry.c
> > @@ -81,6 +81,11 @@ dentry_alloc(struct dentry *parent_dp, struct vnode *vp, 
> > const char *path)
> >     dp->d_vnode = vp;
> >     dp->d_mount = mp;
> >     dp->d_path = strdup(path);
> > +
> > +   if (!dp->d_path) {
> > +           return NULL;
> > +   }
> 
> I think this should be an UK_ASSERT() in the function entry instead if it is
> required to hand-over a path.

I'm not sure to understand this comment... checking the return value of
strdup via UK_ASSERT doesn't sound right to me.

> > +
> >     UK_INIT_LIST_HEAD(&dp->d_child_list);
> >     if (parent_dp) {
> > @@ -133,11 +138,12 @@ static void dentry_children_remove(struct dentry *dp)
> >   }
> > -void
> > +int
> >   dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path)
> >   {
> >     struct dentry *old_pdp = dp->d_parent;
> >     char *old_path = dp->d_path;
> > +   int error = 0;
> >     if (old_pdp) {
> >             uk_mutex_lock(&old_pdp->d_lock);
> > @@ -162,6 +168,9 @@ dentry_move(struct dentry *dp, struct dentry 
> > *parent_dp, char *path)
> >     uk_hlist_del(&dp->d_link);
> >     // Update dp.
> >     dp->d_path = strdup(path);
> > +   if (!dp->d_path)
> > +           error = ENOMEM;
> > +
> 
> Hum... I think it is better to do the strdup() at the function entry and
> return at failure before changing anything to the VFS.

Agree. Done in the v3.

> Btw, do the functions that call dentry_move() expect that the path is newly
> allocated (e.g., by giving something alocated on the stack). I guess yes and
> it would be the better style. I just want to double-check.

Well, as far as I know the path is allocated on the stack (this is src in
the rename function, lib/vfscore/main.c).

> Also, could dp have contain already a path? If so, should that one be freed
> before?

Yes, dp can already have a path, and we typically free it at the end of
dentry_move. However, if we fail to allocate a new path at function entry,
I don't think we should do anything. Just return an error code and let the
caller decide what to do next. That's the most intuitive thing that comes
to my mind.

cheers,
Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel