Re: buffer overflow in vfio_ccw_async_region_write/vfio_ccw_async_region_read

[Eric Farman <farman@linux.ibm.com>]

(Apologies for the delay; I was on holiday when this came in and it was
lost in the noise.)

On Tue, 2022-07-12 at 17:52 +0800, sohu0106 wrote:
> > 
> > In the vfio_ccw_async_region_write/vfio_ccw_async_region_read >
> > function of drivers/s390/cio/vfio_ccw_async.c, parameter "size_t >
> > count" is pass by userland, if "count" is very large, it will
> > bypass > "if (pos + count > sizeof(*region))".(such as
> > count=0xffffffff). Then > it will lead to buffer overflow in
> > "copy_from_user((void *)region + > pos, buf, count)". 

There are some mechanical problems with this patch. It needs a Signed-
off-by tag, and is not applicable in its current form. All easy to
resolve, and documented in: 

Documentation/process/submitting-patches.rst

Having said that, I'm not sure that the problem exists as you describe.
In my tests, submitting a very large count (0xffffffff, per your
example) gets capped to MAX_RW_COUNT [1] before it gets here. If you
have a mechanism that shows otherwise, I'd like to see it so that I can
revisit my own test scenarios. Seeing it fail would imply that the
async region is not the only one affected, and a new version of this
patch would need to also address the io and schib regions in vfio-ccw.

Thanks,
Eric

[1]
https://lore.kernel.org/all/CAADWXX9rrESSEGmA4C9F85E9jo7H-pv+CUtyAU_kyB=DfcHRpA@mail.gmail.com/


> > 
> > 
> > 
> > 
> > diff --git a/vfio_ccw_async.c_org b/vfio_ccw_async.c
> > index 7a838e3..33339ad 100644
> > --- a/vfio_ccw_async.c_org
> > +++ b/vfio_ccw_async.c
> > @@ -21,7 +21,7 @@ static ssize_t vfio_ccw_async_region_read(struct
> > > vfio_ccw_private *private,
> >         struct ccw_cmd_region *region;
> >         int ret;
> > 
> > 
> > -       if (pos + count > sizeof(*region))
> > +       if (pos + count > sizeof(*region) && pos + count > pos)
> >                 return -EINVAL;
> > 
> > 
> >         mutex_lock(&private->io_mutex);
> > @@ -43,7 +43,7 @@ static ssize_t vfio_ccw_async_region_write(struct
> > > vfio_ccw_private *private,
> >         struct ccw_cmd_region *region;
> >         int ret;
> > 
> > 
> > -       if (pos + count > sizeof(*region))
> > +       if (pos + count > sizeof(*region) && pos + count > pos)
> >                 return -EINVAL;
> > 
> > 
> >         if (!mutex_trylock(&private->io_mutex))
> >