Re: [PATCH 10/24] DAX: virtiofsd: Add setup/remove mappings fuse commands

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Feb 11, 2021 at 04:39:22PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > > > +                             struct fuse_mbuf_iter *iter)
> > > > +{
> > > > +    struct fuse_removemapping_in *arg;
> > > > +    struct fuse_removemapping_one *one;
> > > > +
> > > > +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > > +    if (!arg || arg->count <= 0) {
> > > 
> > > arg->count is unsigned so < is tautologous.
> > > 
> > > > +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", 
> > > > arg);
> > > > +        fuse_reply_err(req, EINVAL);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
> > > 
> > > arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
> > > think we should be more defensive here since this input comes from the
> > > guest.
> > 
> > OK, so I've gone with:
> > 
> >     if (!arg || !arg->count || 
> >         (uint64_t)arg->count * sizeof(*one) >= SIZE_MAX) {
> >         fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> >         fuse_reply_err(req, EINVAL);
> >         return;
> 
> If we did not want to get into unit64_t business, can we alternatively do.
>      if (!arg || !arg->count || arg->count > SIZE_MAX/sizeof(*one)) {

I tried that and the compiler moaned that it was always false; which on
a 64bit host it is since arg->count is uint32_t.

Dave

>      }
> 
> Vivek
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK