[PATCH v3 0/3] virtiofsd: fix some accessing NULL pointer problem

[PATCH v3 0/3] virtiofsd: fix some accessing NULL pointer problem

[Haotian Li]

Hi,
  We find some potential NULL pointer bugs on tools/virtiofsd.
Three patches are made to fix them

Haotian Li (3):
  tools/virtiofsd/buffer.c: check whether buf is NULL in
    fuse_bufvec_advance func
  virtiofsd: check whether lo_map_reserve returns NULL in main func
  virtiofsd: check whether strdup lo.source return NULL in main func.

 tools/virtiofsd/buffer.c         |  4 ++++
 tools/virtiofsd/passthrough_ll.c | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

-- 

[PATCH 1/3] tools/virtiofsd/buffer.c: check whether buf is NULL in fuse_bufvec_advance func

[Haotian Li]

In fuse_bufvec_advance func, calling fuse_bufvec_current func
may return NULL, so we should check whether buf is NULL before
using it.

Signed-off-by: Haotian Li <lihaotian9@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 tools/virtiofsd/buffer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c
index 27c1377f22..bdc608c221 100644
--- a/tools/virtiofsd/buffer.c
+++ b/tools/virtiofsd/buffer.c
@@ -246,6 +246,10 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
 {
     const struct fuse_buf *buf = fuse_bufvec_current(bufv);

+    if (!buf) {
+        return 0;
+    }
+
     bufv->off += len;
     assert(bufv->off <= buf->size);
     if (bufv->off == buf->size) {
-- 

Re: [Virtio-fs] [PATCH 1/3] tools/virtiofsd/buffer.c: check whether buf is NULL in fuse_bufvec_advance func

[Dr. David Alan Gilbert]

* Haotian Li (lihaotian9@huawei.com) wrote:
> In fuse_bufvec_advance func, calling fuse_bufvec_current func
> may return NULL, so we should check whether buf is NULL before
> using it.
> 
> Signed-off-by: Haotian Li <lihaotian9@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  tools/virtiofsd/buffer.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c
> index 27c1377f22..bdc608c221 100644
> --- a/tools/virtiofsd/buffer.c
> +++ b/tools/virtiofsd/buffer.c
> @@ -246,6 +246,10 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
>  {
>      const struct fuse_buf *buf = fuse_bufvec_current(bufv);
> 
> +    if (!buf) {
> +        return 0;
> +    }
> +

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>      bufv->off += len;
>      assert(bufv->off <= buf->size);
>      if (bufv->off == buf->size) {
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/3] virtiofsd: check whether lo_map_reserve returns NULL in, main func

[Haotian Li]

In main func, func lo_map_reserve is called without NULL check.
If reallocing new_elems fails in func lo_map_grow, the func
lo_map_reserve may return NULL. We should check whether
lo_map_reserve returns NULL before using it.

Signed-off-by: Haotian Li <lihaotian9@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 tools/virtiofsd/passthrough_ll.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index ec1008bceb..3e9bbc7a04 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3433,6 +3433,7 @@ int main(int argc, char *argv[])
         .proc_self_fd = -1,
     };
     struct lo_map_elem *root_elem;
+    struct lo_map_elem *reserve_elem;
     int ret = -1;

     /* Don't mask creation mode, kernel already did that */
@@ -3452,8 +3453,17 @@ int main(int argc, char *argv[])
      * [1] Root inode
      */
     lo_map_init(&lo.ino_map);
-    lo_map_reserve(&lo.ino_map, 0)->in_use = false;
+    reserve_elem = lo_map_reserve(&lo.ino_map, 0);
+    if (!reserve_elem) {
+        fuse_log(FUSE_LOG_ERR, "failed to alloc reserve_elem.\n");
+        goto err_out1;
+    }
+    reserve_elem->in_use = false;
     root_elem = lo_map_reserve(&lo.ino_map, lo.root.fuse_ino);
+    if (!root_elem) {
+        fuse_log(FUSE_LOG_ERR, "failed to alloc root_elem.\n");
+        goto err_out1;
+    }
     root_elem->inode = &lo.root;

     lo_map_init(&lo.dirp_map);
-- 

Re: [Virtio-fs] [PATCH 2/3] virtiofsd: check whether lo_map_reserve returns NULL in, main func

[Dr. David Alan Gilbert]

* Haotian Li (lihaotian9@huawei.com) wrote:
> In main func, func lo_map_reserve is called without NULL check.
> If reallocing new_elems fails in func lo_map_grow, the func
> lo_map_reserve may return NULL. We should check whether
> lo_map_reserve returns NULL before using it.
> 
> Signed-off-by: Haotian Li <lihaotian9@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(I think the fuse_log will always fall through the default_log_func
becuase it's very early)

> ---
>  tools/virtiofsd/passthrough_ll.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index ec1008bceb..3e9bbc7a04 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3433,6 +3433,7 @@ int main(int argc, char *argv[])
>          .proc_self_fd = -1,
>      };
>      struct lo_map_elem *root_elem;
> +    struct lo_map_elem *reserve_elem;
>      int ret = -1;
> 
>      /* Don't mask creation mode, kernel already did that */
> @@ -3452,8 +3453,17 @@ int main(int argc, char *argv[])
>       * [1] Root inode
>       */
>      lo_map_init(&lo.ino_map);
> -    lo_map_reserve(&lo.ino_map, 0)->in_use = false;
> +    reserve_elem = lo_map_reserve(&lo.ino_map, 0);
> +    if (!reserve_elem) {
> +        fuse_log(FUSE_LOG_ERR, "failed to alloc reserve_elem.\n");
> +        goto err_out1;
> +    }
> +    reserve_elem->in_use = false;
>      root_elem = lo_map_reserve(&lo.ino_map, lo.root.fuse_ino);
> +    if (!root_elem) {
> +        fuse_log(FUSE_LOG_ERR, "failed to alloc root_elem.\n");
> +        goto err_out1;
> +    }
>      root_elem->inode = &lo.root;
> 
>      lo_map_init(&lo.dirp_map);
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 3/3] virtiofsd: check whether strdup lo.source return NULL in main func

[Haotian Li]

In main func, strdup lo.source may fail. So check whether strdup
lo.source return NULL before using it.

Signed-off-by: Haotian Li <lihaotian9@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 tools/virtiofsd/passthrough_ll.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3e9bbc7a04..0c11134fb5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3525,6 +3525,10 @@ int main(int argc, char *argv[])
         }
     } else {
         lo.source = strdup("/");
+        if (!lo.source) {
+            fuse_log(FUSE_LOG_ERR, "failed to strdup source\n");
+            goto err_out1;
+        }
     }

     if (lo.xattrmap) {
-- 

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: check whether strdup lo.source return NULL in main func

[Dr. David Alan Gilbert]

* Haotian Li (lihaotian9@huawei.com) wrote:
> In main func, strdup lo.source may fail. So check whether strdup
> lo.source return NULL before using it.
> 
> Signed-off-by: Haotian Li <lihaotian9@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 3e9bbc7a04..0c11134fb5 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3525,6 +3525,10 @@ int main(int argc, char *argv[])
>          }
>      } else {
>          lo.source = strdup("/");
> +        if (!lo.source) {
> +            fuse_log(FUSE_LOG_ERR, "failed to strdup source\n");
> +            goto err_out1;
> +        }
>      }
> 

(It's interesting we use exit's in some places, goto's in others)

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>      if (lo.xattrmap) {
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 0/3] virtiofsd: fix some accessing NULL pointer problem

[Dr. David Alan Gilbert]

* Haotian Li (lihaotian9@huawei.com) wrote:
> Hi,
>   We find some potential NULL pointer bugs on tools/virtiofsd.
> Three patches are made to fix them

Queued

> Haotian Li (3):
>   tools/virtiofsd/buffer.c: check whether buf is NULL in
>     fuse_bufvec_advance func
>   virtiofsd: check whether lo_map_reserve returns NULL in main func
>   virtiofsd: check whether strdup lo.source return NULL in main func.
> 
>  tools/virtiofsd/buffer.c         |  4 ++++
>  tools/virtiofsd/passthrough_ll.c | 16 +++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> -- 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK