[PATCH] 9pfs: Fix divide by zero bug

[PATCH] 9pfs: Fix divide by zero bug

[Dan Schatzberg]

Some filesystems may return 0s in statfs (trivially, a FUSE filesystem
can do so). QEMU should handle this gracefully and just behave the
same as if statfs failed.

Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
---
 hw/9pfs/9p.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 37abcdb71e..520177f40c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, V9fsPath *path)
      * and as well as less than (client msize - P9_IOHDRSZ))
      */
     if (!v9fs_co_statfs(pdu, path, &stbuf)) {
-        iounit = stbuf.f_bsize;
-        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
+        if (stbuf.f_bsize) {
+            iounit = stbuf.f_bsize;
+            iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize;
+        }
     }
     if (!iounit) {
         iounit = s->msize - P9_IOHDRSZ;
-- 
2.17.1

Re: [PATCH] 9pfs: Fix divide by zero bug

[Christian Schoenebeck]

On Freitag, 22. November 2019 21:00:34 CET Dan Schatzberg wrote:
> Some filesystems may return 0s in statfs (trivially, a FUSE filesystem
> can do so). QEMU should handle this gracefully and just behave the
> same as if statfs failed.

Is that actually legal in non-error cases? Shouldn't a driver without a block 
size concept return 512 according to POSIX?

> Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
> ---
>  hw/9pfs/9p.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 37abcdb71e..520177f40c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu,
> V9fsPath *path) * and as well as less than (client msize - P9_IOHDRSZ))
>       */
>      if (!v9fs_co_statfs(pdu, path, &stbuf)) {
> -        iounit = stbuf.f_bsize;
> -        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> +        if (stbuf.f_bsize) {
> +            iounit = stbuf.f_bsize;
> +            iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize;
> +        }
>      }
>      if (!iounit) {
>          iounit = s->msize - P9_IOHDRSZ;

Nevertheless, since that will leave iounit initialized with zero and since 
there is already an !ionunit case handling there ...

Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Best regards,
Christian Schoenebeck

Re: [PATCH] 9pfs: Fix divide by zero bug

[Greg Kurz]

On Fri, 22 Nov 2019 22:53:09 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 22. November 2019 21:00:34 CET Dan Schatzberg wrote:
> > Some filesystems may return 0s in statfs (trivially, a FUSE filesystem
> > can do so). QEMU should handle this gracefully and just behave the
> > same as if statfs failed.
> 
> Is that actually legal in non-error cases? Shouldn't a driver without a block 
> size concept return 512 according to POSIX?
> 

The first problem I see is that statfs() isn't POSIX. It is a linux-specific
implementation inspired by 4.4BSD. The equivalent system call in POSIX would
be statvfs():

https://pubs.opengroup.org/onlinepubs/9699919799/functions/statvfs.html

And even there, no details are provided about what f_bsize should contain,
apart from:

"It is unspecified whether all members of the statvfs structure have
 meaningful values on all file systems."

A filesystem isn't necessarily backed by a block device, so I guess it
is legal to get a zero block size and we should definitely cope with it.

> > Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
> > ---
> >  hw/9pfs/9p.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 37abcdb71e..520177f40c 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu,
> > V9fsPath *path) * and as well as less than (client msize - P9_IOHDRSZ))
> >       */
> >      if (!v9fs_co_statfs(pdu, path, &stbuf)) {
> > -        iounit = stbuf.f_bsize;
> > -        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> > +        if (stbuf.f_bsize) {
> > +            iounit = stbuf.f_bsize;
> > +            iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize;
> > +        }
> >      }
> >      if (!iounit) {
> >          iounit = s->msize - P9_IOHDRSZ;
> 
> Nevertheless, since that will leave iounit initialized with zero and since 
> there is already an !ionunit case handling there ...
> 

This fix looks like the only sensible thing to do.

> Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 

Thanks for the Cc otherwise I would have certainly missed this one :)

I've pushed it to my 9p-fix branch and I'll send a pull request ASAP
in order to have this fixed in rc3.

> Best regards,
> Christian Schoenebeck
> 
>