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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hi Hugo,

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

Thanks,

Simon

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.
> +
>         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.
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.
Also, could dp have contain already a path? If so, should that one be 
freed before?
>         dp->d_parent = parent_dp;
>         // Insert dp updated hash info into the hashtable.
>         uk_hlist_add_head(&dp->d_link,
> @@ -173,6 +182,7 @@ dentry_move(struct dentry *dp, struct dentry *parent_dp, 
> char *path)
>         }
>   
>   	free(old_path);
> +       return error;
>   }
>   
>   void
> diff --git a/lib/vfscore/include/vfscore/dentry.h 
> b/lib/vfscore/include/vfscore/dentry.h
> index 0a38402..2c35653 100644
> --- a/lib/vfscore/include/vfscore/dentry.h
> +++ b/lib/vfscore/include/vfscore/dentry.h
> @@ -56,7 +56,7 @@ struct dentry {
>   
>   struct dentry *dentry_alloc(struct dentry *parent_dp, struct vnode *vp, const char *path);
>   struct dentry *dentry_lookup(struct mount *mp, char *path);
> -void dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path);
> +int dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path);
>   void dentry_remove(struct dentry *dp);
>   void dref(struct dentry *dp);
>   void drele(struct dentry *dp);
> diff --git a/lib/vfscore/syscalls.c b/lib/vfscore/syscalls.c
> index 9b5a6bd..9a132b7 100644
> --- a/lib/vfscore/syscalls.c
> +++ b/lib/vfscore/syscalls.c
> @@ -833,8 +833,11 @@ sys_rename(char *src, char *dest)
>         }
>   
>   	error = VOP_RENAME(dvp1, vp1, sname, dvp2, vp2, dname);
> +       if (error)
> +               goto err3;
> +
> +       error = dentry_move(dp1, ddp2, dname);
>   
> -	dentry_move(dp1, ddp2, dname);
>         if (dp2)
>                 dentry_remove(dp2);
>   
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel