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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Great. Thanks a lot for your work!

Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>

On 06.04.20 16:05, 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>
> ---
> Changes since v2:
>   + dentry_alloc: free(dp) if strdup returns NULL, otherwise this is a memory 
> leak.
>
> diff --git a/lib/vfscore/dentry.c b/lib/vfscore/dentry.c
> index 76f7a6b..43ee2fe 100644
> --- a/lib/vfscore/dentry.c
> +++ b/lib/vfscore/dentry.c
> @@ -75,12 +75,17 @@ dentry_alloc(struct dentry *parent_dp, struct vnode *vp, 
> const char *path)
>                 return NULL;
>         }
>   
> +	dp->d_path = strdup(path);
> +       if (!dp->d_path) {
> +               free(dp);
> +               return NULL;
> +       }
> +
>         vref(vp);
>   
>   	dp->d_refcnt = 1;
>         dp->d_vnode = vp;
>         dp->d_mount = mp;
> -       dp->d_path = strdup(path);
>         UK_INIT_LIST_HEAD(&dp->d_child_list);
>   
>   	if (parent_dp) {
> @@ -133,11 +138,17 @@ 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;
> +       char *new_path = strdup(path);
> +
> +       if (!new_path) {
> +               // Fail before changing anything to the VFS
> +               return ENOMEM;
> +       }
>   
>   	if (old_pdp) {
>                 uk_mutex_lock(&old_pdp->d_lock);
> @@ -161,7 +172,8 @@ dentry_move(struct dentry *dp, struct dentry *parent_dp, 
> char *path)
>         // Remove dp with outdated hash info from the hashtable.
>         uk_hlist_del(&dp->d_link);
>         // Update dp.
> -       dp->d_path = strdup(path);
> +       dp->d_path = new_path;
> +
>         dp->d_parent = parent_dp;
>         // Insert dp updated hash info into the hashtable.
>         uk_hlist_add_head(&dp->d_link,
> @@ -173,6 +185,7 @@ dentry_move(struct dentry *dp, struct dentry *parent_dp, 
> char *path)
>         }
>   
>   	free(old_path);
> +       return 0;
>   }
>   
>   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);
>