[PATCH] vfio/qat: fix f_pos race in qat_vf_resume_write()

[PATCH] vfio/qat: fix f_pos race in qat_vf_resume_write()

[Giovanni Cabiddu]

qat_vf_resume_write() checks filp->f_pos before taking migf->lock, but
copies into the migration-state buffer after taking the lock and
re-reading the shared file position.

Two concurrent writers could therefore pass the bounds check with the
old offset, then have the second writer copy after the first advanced
f_pos, writing past the end of the migration-state buffer.

Take migf->lock before doing the boundary checks.

Fixes: bb208810b1ab ("vfio/qat: Add vfio_pci driver for Intel QAT SR-IOV VF devices")
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Reviewed-by: Ahsan Atta <ahsan.atta@intel.com>
---
 drivers/vfio/pci/qat/main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/pci/qat/main.c b/drivers/vfio/pci/qat/main.c
index ac9652539d66..60ff907b6a67 100644
--- a/drivers/vfio/pci/qat/main.c
+++ b/drivers/vfio/pci/qat/main.c
@@ -298,14 +298,18 @@ static ssize_t qat_vf_resume_write(struct file *filp, const char __user *buf,
 		return -ESPIPE;
 	offs = &filp->f_pos;
 
-	if (*offs < 0 ||
-	    check_add_overflow(len, *offs, &end))
-		return -EOVERFLOW;
-
-	if (end > mig_dev->state_size)
-		return -ENOMEM;
-
 	mutex_lock(&migf->lock);
+
+	if (*offs < 0 || check_add_overflow(len, *offs, &end)) {
+		done = -EOVERFLOW;
+		goto out_unlock;
+	}
+
+	if (end > mig_dev->state_size) {
+		done = -ENOMEM;
+		goto out_unlock;
+	}
+
 	if (migf->disabled) {
 		done = -ENODEV;
 		goto out_unlock;
-- 
2.54.0

Re: [PATCH] vfio/qat: fix f_pos race in qat_vf_resume_write()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Time-Of-Check to Time-Of-Use (TOCTOU) race condition on filp->f_pos leading to a heap Out-Of-Bounds (OOB) write.
--

commit 1308b8d3699cdbd06dda3a3c256a63827fa74e14
Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>

vfio/qat: fix f_pos race in qat_vf_resume_write()

This commit fixes a race condition in qat_vf_resume_write() where bounds
checking on f_pos was performed outside of the migration lock. By moving
the bounds checks inside the lock, it prevents concurrent writers from
using an old file offset to bypass boundaries and copy past the end of the
migration-state buffer.

> diff --git a/drivers/vfio/pci/qat/main.c b/drivers/vfio/pci/qat/main.c
> index ac9652539d66a..60ff907b6a67a 100644
> --- a/drivers/vfio/pci/qat/main.c
> +++ b/drivers/vfio/pci/qat/main.c
> @@ -298,14 +298,18 @@ static ssize_t qat_vf_resume_write(struct file *filp, const char __user *buf,
>  		return -ESPIPE;
>  	offs = &filp->f_pos;
>  
> -	if (*offs < 0 ||
> -	    check_add_overflow(len, *offs, &end))
> -		return -EOVERFLOW;
> +	mutex_lock(&migf->lock);

[Severity: Critical]
This is a pre-existing issue, but does this same regression exist in other
VFIO drivers?

Since migration file descriptors opened by VFIO drivers use stream_open()
which unsets FMODE_ATOMIC_POS, the VFS does not acquire f_pos_lock when
userspace calls write().

This means concurrent writes can evaluate the initial f_pos before the
mutex, and then use an advanced f_pos inside copy_from_user(), leading to a
heap out-of-bounds write.

For example, this identical pattern appears in
hisi_acc_vfio_pci.c:hisi_acc_vf_resume_write():

	pos = &filp->f_pos;

	if (*pos < 0 ||
	    check_add_overflow((loff_t)len, *pos, &requested_length))
		return -EINVAL;

	if (requested_length > sizeof(struct acc_vf_data))
		return -ENOMEM;

	mutex_lock(&migf->lock);
	// ...
	ret = copy_from_user(vf_data + *pos, buf, len);

And also in samples/vfio-mdev/mtty.c:mtty_resume_write():

	pos = &filp->f_pos;

	if (*pos < 0 ||
	    check_add_overflow((loff_t)len, *pos, &requested_length))
		return -EINVAL;

	if (requested_length > mtty_data_size(mdev_state))
		return -ENOMEM;

	mutex_lock(&migf->lock);
	// ...
	if (copy_from_user((void *)&migf->data + *pos, buf, len)) {

Should these other drivers be patched similarly?

> -	if (end > mig_dev->state_size)
> -		return -ENOMEM;
> +	if (*offs < 0 || check_add_overflow(len, *offs, &end)) {
> +		done = -EOVERFLOW;
> +		goto out_unlock;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608151317.136613-1-giovanni.cabiddu@intel.com?part=1