| Message ID | YsP+2CWqMudArkqF@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: hisi_acc_vfio_pci: fix integer overflow check in hisi_acc_vf_resume_write() | expand |
On Tue, Jul 05, 2022 at 12:05:28PM +0300, Dan Carpenter wrote: > The casting on this makes the integer overflow check slightly wrong. > "len" is an unsigned long. "*pos" and "requested_length" are signed > long longs. Imagine "len" is ULONG_MAX and "*pos" is 2. > "ULONG_MAX + 2 = 1". I wonder if this can happen, len is a kernel controlled value bounded by a memory allocation.. > Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- This code was copy and pasted from drivers/vfio/pci/mlx5/main.c, so it should be fixed too > It is strange that we are doing: > > pos = &filp->f_pos; > > instead of using the passed in value of pos. IIRC the way we have the struct file configured the pos argument is NULL. Jason
On Tue, Jul 05, 2022 at 03:06:49PM -0300, Jason Gunthorpe wrote: > On Tue, Jul 05, 2022 at 12:05:28PM +0300, Dan Carpenter wrote: > > The casting on this makes the integer overflow check slightly wrong. > > "len" is an unsigned long. "*pos" and "requested_length" are signed > > long longs. Imagine "len" is ULONG_MAX and "*pos" is 2. > > "ULONG_MAX + 2 = 1". > > I wonder if this can happen, len is a kernel controlled value bounded > by a memory allocation.. > Oh. Smatch uses a model which says that all read/writes come from vfs_write(). The problem with tracking kernel read/writes is that recursion is tricky. So Smatch just deletes those from the DB. > > Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > This code was copy and pasted from drivers/vfio/pci/mlx5/main.c, so it > should be fixed too Sure. I created a static checker warning for this type of thing but it didn't catch the issue in drivers/vfio/pci/mlx5/main.c because Smatch says that the bug is impossible. Which is true. Smatch doesn't really parse rw_verify_area() accurately. I just hard coded that function as accepting values 0-1000000000 for both *ppos and count. regards, dan carpenter
On Wed, Jul 06, 2022 at 08:51:24AM +0300, Dan Carpenter wrote: > On Tue, Jul 05, 2022 at 03:06:49PM -0300, Jason Gunthorpe wrote: > > On Tue, Jul 05, 2022 at 12:05:28PM +0300, Dan Carpenter wrote: > > > The casting on this makes the integer overflow check slightly wrong. > > > "len" is an unsigned long. "*pos" and "requested_length" are signed > > > long longs. Imagine "len" is ULONG_MAX and "*pos" is 2. > > > "ULONG_MAX + 2 = 1". > > > > I wonder if this can happen, len is a kernel controlled value bounded > > by a memory allocation.. > > > > Oh. Smatch uses a model which says that all read/writes come from > vfs_write(). The problem with tracking kernel read/writes is that > recursion is tricky. So Smatch just deletes those from the DB. Oh, maybe I got it wrong, len is the user input, so yes that does look bad > > This code was copy and pasted from drivers/vfio/pci/mlx5/main.c, so it > > should be fixed too > > Sure. > > I created a static checker warning for this type of thing but it didn't > catch the issue in drivers/vfio/pci/mlx5/main.c because Smatch says that > the bug is impossible. Which is true. How come it is different? Jason
On Wed, Jul 06, 2022 at 01:18:12PM -0300, Jason Gunthorpe wrote: > On Wed, Jul 06, 2022 at 08:51:24AM +0300, Dan Carpenter wrote: > > > > This code was copy and pasted from drivers/vfio/pci/mlx5/main.c, so it > > > should be fixed too > > > > Sure. > > > > I created a static checker warning for this type of thing but it didn't > > catch the issue in drivers/vfio/pci/mlx5/main.c because Smatch says that > > the bug is impossible. Which is true. > > How come it is different? No, it doesn't find either one. I don't think it's a real bug because of the rw_verify_area() thing. But I wrote the check based on noticing it during review just to see if there were similar issues and didn't find anything. regards, dan carpenter
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index ea762e28c1cc..dcc34488b0c0 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -701,7 +701,7 @@ static ssize_t hisi_acc_vf_resume_write(struct file *filp, const char __user *bu size_t len, loff_t *pos) { struct hisi_acc_vf_migration_file *migf = filp->private_data; - loff_t requested_length; + unsigned long requested_length; ssize_t done = 0; int ret; @@ -709,8 +709,8 @@ static ssize_t hisi_acc_vf_resume_write(struct file *filp, const char __user *bu return -ESPIPE; pos = &filp->f_pos; - if (*pos < 0 || - check_add_overflow((loff_t)len, *pos, &requested_length)) + if (*pos < 0 || *pos > ULONG_MAX || + check_add_overflow(len, (unsigned long)*pos, &requested_length)) return -EINVAL; if (requested_length > sizeof(struct acc_vf_data))
The casting on this makes the integer overflow check slightly wrong. "len" is an unsigned long. "*pos" and "requested_length" are signed long longs. Imagine "len" is ULONG_MAX and "*pos" is 2. "ULONG_MAX + 2 = 1". That's an integer overflow. However, if we cast the ULONG_MAX to long long then "-1 + 2 = 1". That's not an integer overflow. It's simpler if "requested_length" length is an unsigned value so we don't have to worry about negatives. I believe that the checks in the VFS layer and the check for "*pos < 0" probably prevent this bug in real life, but it's safer to just be sure. Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- It is strange that we are doing: pos = &filp->f_pos; instead of using the passed in value of pos. The VFS layer ensures that the passed in value of "*pos + len" cannot overflow in rw_verify_area() so normally this check could have been removed. drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)