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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 06.04.20 16:09, Hugo Lefeuvre wrote:
> 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.
Ah, sorry, I missed the assignment in the declaration block. You are right.

> > > +
> > >         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.
All fine. I had the same issue here: I missed the copy in the 
declaration block (old_path); you are right! ;-)
> cheers,
> Hugo
Thanks,

Simon

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