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 <[email protected]>
---
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(-)
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))
--
2.35.1
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 <[email protected]>
> ---
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 <[email protected]>
> > ---
>
> 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