* [patch 1/3] vfio: signedness bug in vfio_config_do_rw()
[not found] <1340686552.1207.128.camel@bling.home>
@ 2012-06-28 6:44 ` Dan Carpenter
2012-06-28 7:15 ` walter harms
2012-06-28 8:05 ` [patch 1/3] " Dan Carpenter
2012-06-28 6:44 ` [patch 2/3] vfio: make count unsigned to prevent integer underflow Dan Carpenter
2012-06-28 6:45 ` [patch 3/3] vfio: return -EFAULT on failure Dan Carpenter
2 siblings, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2012-06-28 6:44 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, linux-kernel, kernel-janitors
The "count" variable is unsigned here so the test for errors doesn't
work.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index a4f7321..10bc6a8 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1487,7 +1487,7 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_device *vdev, char __user *buf,
if (perm->readfn) {
count = perm->readfn(vdev, *ppos, count,
perm, offset, &val);
- if (count < 0)
+ if ((ssize_t)count < 0)
return count;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch 1/3] vfio: signedness bug in vfio_config_do_rw()
2012-06-28 6:44 ` [patch 1/3] vfio: signedness bug in vfio_config_do_rw() Dan Carpenter
@ 2012-06-28 7:15 ` walter harms
2012-06-28 8:07 ` [patch 1/3 v2] " Dan Carpenter
2012-06-28 8:05 ` [patch 1/3] " Dan Carpenter
1 sibling, 1 reply; 9+ messages in thread
From: walter harms @ 2012-06-28 7:15 UTC (permalink / raw)
To: kernel-janitors
Am 28.06.2012 08:44, schrieb Dan Carpenter:
> The "count" variable is unsigned here so the test for errors doesn't
> work.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a4f7321..10bc6a8 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1487,7 +1487,7 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_device *vdev, char __user *buf,
> if (perm->readfn) {
> count = perm->readfn(vdev, *ppos, count,
> perm, offset, &val);
> - if (count < 0)
> + if ((ssize_t)count < 0)
> return count;
> }
>
hi,
I can only find a few references to vfio_config_do_rw(). From the patchit seems to me this is a wrapper
for perm->readfn since both return ssize_t so why i count unsigned in the first place ?
re,
wh
^ permalink raw reply [flat|nested] 9+ messages in thread* [patch 1/3 v2] vfio: signedness bug in vfio_config_do_rw()
2012-06-28 7:15 ` walter harms
@ 2012-06-28 8:07 ` Dan Carpenter
2012-06-28 22:24 ` Alex Williamson
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2012-06-28 8:07 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, linux-kernel, kernel-janitors, walter harms
The "count" variable needs to be signed here because we use it to store
negative error codes.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Just declare count as signed.
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index a4f7321..2e00aa8 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1419,7 +1419,7 @@ void vfio_config_free(struct vfio_pci_device *vdev)
}
static ssize_t vfio_config_do_rw(struct vfio_pci_device *vdev, char __user *buf,
- size_t count, loff_t *ppos, bool iswrite)
+ ssize_t count, loff_t *ppos, bool iswrite)
{
struct pci_dev *pdev = vdev->pdev;
struct perm_bits *perm;
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch 1/3 v2] vfio: signedness bug in vfio_config_do_rw()
2012-06-28 8:07 ` [patch 1/3 v2] " Dan Carpenter
@ 2012-06-28 22:24 ` Alex Williamson
0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2012-06-28 22:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kvm, linux-kernel, kernel-janitors, walter harms
On Thu, 2012-06-28 at 11:07 +0300, Dan Carpenter wrote:
> The "count" variable needs to be signed here because we use it to store
> negative error codes.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Just declare count as signed.
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a4f7321..2e00aa8 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1419,7 +1419,7 @@ void vfio_config_free(struct vfio_pci_device *vdev)
> }
>
> static ssize_t vfio_config_do_rw(struct vfio_pci_device *vdev, char __user *buf,
> - size_t count, loff_t *ppos, bool iswrite)
> + ssize_t count, loff_t *ppos, bool iswrite)
> {
> struct pci_dev *pdev = vdev->pdev;
> struct perm_bits *perm;
signed doesn't seem right for count since just below this chunk we do:
if (*ppos < 0 || *ppos + count > pdev->cfg_size)
return -EFAULT;
So then we have to start testing for negative count. I've added a
ssize_t variable for return that should clear things up. Thanks for the
report!
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 1/3] vfio: signedness bug in vfio_config_do_rw()
2012-06-28 6:44 ` [patch 1/3] vfio: signedness bug in vfio_config_do_rw() Dan Carpenter
2012-06-28 7:15 ` walter harms
@ 2012-06-28 8:05 ` Dan Carpenter
1 sibling, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2012-06-28 8:05 UTC (permalink / raw)
To: kernel-janitors
On Thu, Jun 28, 2012 at 09:15:12AM +0200, walter harms wrote:
>
>
> Am 28.06.2012 08:44, schrieb Dan Carpenter:
> > The "count" variable is unsigned here so the test for errors doesn't
> > work.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index a4f7321..10bc6a8 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -1487,7 +1487,7 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_device *vdev, char __user *buf,
> > if (perm->readfn) {
> > count = perm->readfn(vdev, *ppos, count,
> > perm, offset, &val);
> > - if (count < 0)
> > + if ((ssize_t)count < 0)
> > return count;
> > }
> >
>
> hi,
> I can only find a few references to vfio_config_do_rw(). From the
> patchit seems to me this is a wrapper for perm->readfn since both
> return ssize_t so why i count unsigned in the first place ?
Yeah. You're right. That was stupid of me. I'll resend.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch 2/3] vfio: make count unsigned to prevent integer underflow
[not found] <1340686552.1207.128.camel@bling.home>
2012-06-28 6:44 ` [patch 1/3] vfio: signedness bug in vfio_config_do_rw() Dan Carpenter
@ 2012-06-28 6:44 ` Dan Carpenter
2012-06-28 22:24 ` Alex Williamson
2012-06-28 6:45 ` [patch 3/3] vfio: return -EFAULT on failure Dan Carpenter
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2012-06-28 6:44 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, linux-kernel, kernel-janitors
In vfio_pci_ioctl() there is a potential integer underflow where we
might allocate less data than intended. We check that hdr.count is not
too large, but we don't check whether it is negative:
drivers/vfio/pci/vfio_pci.c
312 if (hdr.argsz - minsz < hdr.count * size ||
313 hdr.count > vfio_pci_get_irq_count(vdev, hdr.index))
314 return -EINVAL;
315
316 data = kmalloc(hdr.count * size, GFP_KERNEL);
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 300d49b..86ef2da 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -347,7 +347,7 @@ struct vfio_irq_set {
#define VFIO_IRQ_SET_ACTION_TRIGGER (1 << 5) /* Trigger interrupt */
__u32 index;
__s32 start;
- __s32 count;
+ __u32 count;
__u8 data[];
};
#define VFIO_DEVICE_SET_IRQS _IO(VFIO_TYPE, VFIO_BASE + 10)
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch 2/3] vfio: make count unsigned to prevent integer underflow
2012-06-28 6:44 ` [patch 2/3] vfio: make count unsigned to prevent integer underflow Dan Carpenter
@ 2012-06-28 22:24 ` Alex Williamson
0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2012-06-28 22:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kvm, linux-kernel, kernel-janitors
On Thu, 2012-06-28 at 09:44 +0300, Dan Carpenter wrote:
> In vfio_pci_ioctl() there is a potential integer underflow where we
> might allocate less data than intended. We check that hdr.count is not
> too large, but we don't check whether it is negative:
>
> drivers/vfio/pci/vfio_pci.c
> 312 if (hdr.argsz - minsz < hdr.count * size ||
> 313 hdr.count > vfio_pci_get_irq_count(vdev, hdr.index))
> 314 return -EINVAL;
> 315
> 316 data = kmalloc(hdr.count * size, GFP_KERNEL);
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 300d49b..86ef2da 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -347,7 +347,7 @@ struct vfio_irq_set {
> #define VFIO_IRQ_SET_ACTION_TRIGGER (1 << 5) /* Trigger interrupt */
> __u32 index;
> __s32 start;
> - __s32 count;
> + __u32 count;
> __u8 data[];
> };
> #define VFIO_DEVICE_SET_IRQS _IO(VFIO_TYPE, VFIO_BASE + 10)
Good find. I've actually trickled this through to change a number of
the function params to unsigned from int. Also in this struct, start
should be unsigned. Thanks for the report!
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch 3/3] vfio: return -EFAULT on failure
[not found] <1340686552.1207.128.camel@bling.home>
2012-06-28 6:44 ` [patch 1/3] vfio: signedness bug in vfio_config_do_rw() Dan Carpenter
2012-06-28 6:44 ` [patch 2/3] vfio: make count unsigned to prevent integer underflow Dan Carpenter
@ 2012-06-28 6:45 ` Dan Carpenter
2012-06-28 22:25 ` Alex Williamson
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2012-06-28 6:45 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, linux-kernel, kernel-janitors
This ioctl function is supposed to return a negative error code or zero
on success. copy_to_user() returns zero or the number of bytes
remaining to be copied.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 457acf3..1aa373f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1159,6 +1159,8 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET;
ret = copy_to_user((void __user *)arg, &status, minsz);
+ if (ret)
+ ret = -EFAULT;
break;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch 3/3] vfio: return -EFAULT on failure
2012-06-28 6:45 ` [patch 3/3] vfio: return -EFAULT on failure Dan Carpenter
@ 2012-06-28 22:25 ` Alex Williamson
0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2012-06-28 22:25 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kvm, linux-kernel, kernel-janitors
On Thu, 2012-06-28 at 09:45 +0300, Dan Carpenter wrote:
> This ioctl function is supposed to return a negative error code or zero
> on success. copy_to_user() returns zero or the number of bytes
> remaining to be copied.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 457acf3..1aa373f 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1159,6 +1159,8 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
> status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET;
>
> ret = copy_to_user((void __user *)arg, &status, minsz);
> + if (ret)
> + ret = -EFAULT;
>
> break;
> }
Yes, thank you! I've folded all of these into the commits on my next
branch, so they should be cleaned up in tomorrow's tree. Thanks for the
reports, please let me know if you find more.
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread