[PATCH] comedi: Fix race condition when updating struct comedi_file

[Ian Abbott]

struct comedi_file holds per file object settings for comedi devices to
allow user space to change the "read" and "write" comedi subdevices
locally to the open file description, using the COMEDI_SETWSUBD and
COMEDI_SETWSUBD ioctl commands.  If there are any subsequent changes to
the status of the attachment of the comedi device to a low-level comedi
driver, the references to the "read" and "write" subdevices in the
struct comedi_file become stale and need to be reset to the current
defaults for the comedi device.  That is done by comedi_file_reset()
when comedi_file_check() (called by comedi_file_read_subdevice() and
comedi_file_write_subdevice()) has determined that they have become
stale.

The tasks calling these functions will have either locked the main mutex
for the comedi device, or will have read-locked the "attach_lock" for
the comedi device.  In the latter case, there may be several tasks
executing the functions at the same time.  Unfortunately, there is a
race condition in the access to the struct comedi_file members that can
leave it having stale pointers to "read" and/or "write" subdevices.
Reorder the accesses to the struct comedi_file members and add SMP
memory barriers to fix the problem.

Fixes: 20f083c07565 ("staging: comedi: prepare support for per-file read and write subdevices")
Reported-by: Eulgyu Kim <eulgyukim@xxxxxxxxx>
Reported-by: Jaeyoung Chung <jjy600901@xxxxxxxxx>
Closes: https://lore.kernel.org/lkml/20260819174614.3857878-1-jjy600901@xxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
---
 drivers/comedi/comedi_fops.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 68eddf853465b..069656bab7e2c 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -66,7 +66,7 @@ struct comedi_file {
 	struct comedi_subdevice *read_subdev;
 	struct comedi_subdevice *write_subdev;
 	unsigned int last_detach_count;
-	unsigned int last_attached:1;
+	unsigned char last_attached;
 };
 
 #define COMEDI_NUM_MINORS 0x100
@@ -293,10 +293,17 @@ static void comedi_file_reset(struct file *file)
 		if (!s || s->subdev_flags & SDF_CMD_WRITE)
 			write_s = s;
 	}
-	cfp->last_attached = dev->attached;
-	cfp->last_detach_count = dev->detach_count;
 	WRITE_ONCE(cfp->read_subdev, read_s);
 	WRITE_ONCE(cfp->write_subdev, write_s);
+	/*
+	 * All tasks that call this function simultaneously will store the same
+	 * values in the cfp members.  The last_attached and last_detach_count
+	 * members will be checked for not being stale before assuming the
+	 * read_subdev and write_subdev members are valid.
+	 */
+	smp_wmb();	/* matched with smp_rmb() in comedi_file_check() */
+	WRITE_ONCE(cfp->last_attached, dev->attached);
+	WRITE_ONCE(cfp->last_detach_count, dev->detach_count);
 }
 
 static void comedi_file_check(struct file *file)
@@ -304,9 +311,11 @@ static void comedi_file_check(struct file *file)
 	struct comedi_file *cfp = file->private_data;
 	struct comedi_device *dev = cfp->dev;
 
-	if (cfp->last_attached != dev->attached ||
-	    cfp->last_detach_count != dev->detach_count)
+	if (READ_ONCE(cfp->last_attached) != dev->attached ||
+	    READ_ONCE(cfp->last_detach_count) != dev->detach_count)
 		comedi_file_reset(file);
+	/* caller is about to load cfp->read_subdev or cfp->write_subdev */
+	smp_rmb();
 }
 
 static struct comedi_subdevice *comedi_file_read_subdevice(struct file *file)
@@ -2227,6 +2236,8 @@ static int do_setrsubd_ioctl(struct comedi_device *dev, unsigned long arg,
 		return -EBUSY;
 
 	WRITE_ONCE(cfp->read_subdev, s_new);
+	/* comedi_file_read_subdevice() => comedi_file_check() => smp_rmb() */
+	smp_wmb();
 	return 0;
 }
 
@@ -2270,6 +2281,8 @@ static int do_setwsubd_ioctl(struct comedi_device *dev, unsigned long arg,
 		return -EBUSY;
 
 	WRITE_ONCE(cfp->write_subdev, s_new);
+	/* comedi_file_write_subdevice() => comedi_file_check() => smp_rmb() */
+	smp_wmb();
 	return 0;
 }
 
-- 
2.53.0