[PATCH] comedi: include poll wait state in busy assessment

[PATCH] comedi: include poll wait state in busy assessment

[Jens Axboe]

syzbot reports a use-after-free in comedi in the below link, which is
due to comedi gladly removing the allocated async area even though poll
requests are still active on the wait_queue_head inside of it. This can
cause a use-after-free when the poll entries are later triggered or
removed, as the memory for the wait_queue_head has been freed. The
notion of being busy in comedi seems mostly centered around sync
syscalls, where the comedi subdevice stores the file in 'busy' as a
marker. This is obviously broken for things like poll which can be
persistent across syscalls.

Rename is_device_busy() to start_detach(), and move it under the
dev->attach_lock. The latter serializes it with poll attempts. If
start_detach() is successful, then it will have marked the device with
->detaching == 1 and this will prevent further poll attempts.

Similarly, have start_detach() check for active polls on the device, and
return busy for that case.

Cc: stable@vger.kernel.org
Fixes: 8ffdff6a8cfb ("staging: comedi: move out of staging directory")
Link: https://lore.kernel.org/all/687bd5fe.a70a0220.693ce.0091.GAE@google.com/
Reported-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
Tested-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index c83fd14dd7ad..58b034e45283 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -782,24 +782,33 @@ void comedi_device_cancel_all(struct comedi_device *dev)
 	}
 }
 
-static int is_device_busy(struct comedi_device *dev)
+static int start_detach(struct comedi_device *dev)
 {
 	struct comedi_subdevice *s;
-	int i;
+	int i, is_busy = 0;
 
 	lockdep_assert_held(&dev->mutex);
+	lockdep_assert_held(&dev->attach_lock);
 	if (!dev->attached)
 		return 0;
 
 	for (i = 0; i < dev->n_subdevices; i++) {
 		s = &dev->subdevices[i];
-		if (s->busy)
-			return 1;
-		if (s->async && comedi_buf_is_mmapped(s))
-			return 1;
+		if (s->busy) {
+			is_busy = 1;
+			break;
+		}
+		if (!s->async)
+			continue;
+		if (comedi_buf_is_mmapped(s) ||
+		    wq_has_sleeper(&s->async->wait_head)) {
+			is_busy = 1;
+			break;
+		}
 	}
-
-	return 0;
+	if (!is_busy)
+		dev->detaching = 1;
+	return is_busy;
 }
 
 /*
@@ -825,8 +834,13 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
 		return -EPERM;
 
 	if (!arg) {
-		if (is_device_busy(dev))
+		/* prevent new polls */
+		down_write(&dev->attach_lock);
+		if (start_detach(dev)) {
+			up_write(&dev->attach_lock);
 			return -EBUSY;
+		}
+		up_write(&dev->attach_lock);
 		if (dev->attached) {
 			struct module *driver_module = dev->driver->module;
 
@@ -2479,7 +2493,7 @@ static __poll_t comedi_poll(struct file *file, poll_table *wait)
 
 	down_read(&dev->attach_lock);
 
-	if (!dev->attached) {
+	if (!dev->attached || dev->detaching) {
 		dev_dbg(dev->class_dev, "no driver attached\n");
 		goto done;
 	}
diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h
index 4cb0400ad616..b2bec668785f 100644
--- a/include/linux/comedi/comedidev.h
+++ b/include/linux/comedi/comedidev.h
@@ -545,6 +545,7 @@ struct comedi_device {
 	const char *board_name;
 	const void *board_ptr;
 	unsigned int attached:1;
+	unsigned int detaching:1;
 	unsigned int ioenabled:1;
 	spinlock_t spinlock;	/* generic spin-lock for low-level driver */
 	struct mutex mutex;	/* generic mutex for COMEDI core */

-- 
Jens Axboe

Re: [PATCH] comedi: include poll wait state in busy assessment

[Ian Abbott]

On 22/07/2025 13:49, Jens Axboe wrote:
> syzbot reports a use-after-free in comedi in the below link, which is
> due to comedi gladly removing the allocated async area even though poll
> requests are still active on the wait_queue_head inside of it. This can
> cause a use-after-free when the poll entries are later triggered or
> removed, as the memory for the wait_queue_head has been freed. The
> notion of being busy in comedi seems mostly centered around sync
> syscalls, where the comedi subdevice stores the file in 'busy' as a
> marker. This is obviously broken for things like poll which can be
> persistent across syscalls.
> 
> Rename is_device_busy() to start_detach(), and move it under the
> dev->attach_lock. The latter serializes it with poll attempts. If
> start_detach() is successful, then it will have marked the device with
> ->detaching == 1 and this will prevent further poll attempts.
> 
> Similarly, have start_detach() check for active polls on the device, and
> return busy for that case.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8ffdff6a8cfb ("staging: comedi: move out of staging directory")
> Link: https://lore.kernel.org/all/687bd5fe.a70a0220.693ce.0091.GAE@google.com/
> Reported-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
> Tested-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> index c83fd14dd7ad..58b034e45283 100644
> --- a/drivers/comedi/comedi_fops.c
> +++ b/drivers/comedi/comedi_fops.c
> @@ -782,24 +782,33 @@ void comedi_device_cancel_all(struct comedi_device *dev)
>   	}
>   }
>   
> -static int is_device_busy(struct comedi_device *dev)
> +static int start_detach(struct comedi_device *dev)
>   {
>   	struct comedi_subdevice *s;
> -	int i;
> +	int i, is_busy = 0;
>   
>   	lockdep_assert_held(&dev->mutex);
> +	lockdep_assert_held(&dev->attach_lock);
>   	if (!dev->attached)
>   		return 0;
>   
>   	for (i = 0; i < dev->n_subdevices; i++) {
>   		s = &dev->subdevices[i];
> -		if (s->busy)
> -			return 1;
> -		if (s->async && comedi_buf_is_mmapped(s))
> -			return 1;
> +		if (s->busy) {
> +			is_busy = 1;
> +			break;
> +		}
> +		if (!s->async)
> +			continue;
> +		if (comedi_buf_is_mmapped(s) ||
> +		    wq_has_sleeper(&s->async->wait_head)) {
> +			is_busy = 1;
> +			break;
> +		}
>   	}
> -
> -	return 0;
> +	if (!is_busy)
> +		dev->detaching = 1;
> +	return is_busy;
>   }
>   
>   /*
> @@ -825,8 +834,13 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
>   		return -EPERM;
>   
>   	if (!arg) {
> -		if (is_device_busy(dev))
> +		/* prevent new polls */
> +		down_write(&dev->attach_lock);
> +		if (start_detach(dev)) {
> +			up_write(&dev->attach_lock);
>   			return -EBUSY;
> +		}
> +		up_write(&dev->attach_lock);
>   		if (dev->attached) {
>   			struct module *driver_module = dev->driver->module;
>   
> @@ -2479,7 +2493,7 @@ static __poll_t comedi_poll(struct file *file, poll_table *wait)
>   
>   	down_read(&dev->attach_lock);
>   
> -	if (!dev->attached) {
> +	if (!dev->attached || dev->detaching) {
>   		dev_dbg(dev->class_dev, "no driver attached\n");
>   		goto done;
>   	}
> diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h
> index 4cb0400ad616..b2bec668785f 100644
> --- a/include/linux/comedi/comedidev.h
> +++ b/include/linux/comedi/comedidev.h
> @@ -545,6 +545,7 @@ struct comedi_device {
>   	const char *board_name;
>   	const void *board_ptr;
>   	unsigned int attached:1;
> +	unsigned int detaching:1;
>   	unsigned int ioenabled:1;
>   	spinlock_t spinlock;	/* generic spin-lock for low-level driver */
>   	struct mutex mutex;	/* generic mutex for COMEDI core */
> 

Thanks for the patch.  I'll post my version shortly.  One problem with 
this patch is that dev->detaching does not get cleared.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH] comedi: include poll wait state in busy assessment

[Jens Axboe]

On 7/22/25 9:29 AM, Ian Abbott wrote:
> On 22/07/2025 13:49, Jens Axboe wrote:
>> syzbot reports a use-after-free in comedi in the below link, which is
>> due to comedi gladly removing the allocated async area even though poll
>> requests are still active on the wait_queue_head inside of it. This can
>> cause a use-after-free when the poll entries are later triggered or
>> removed, as the memory for the wait_queue_head has been freed. The
>> notion of being busy in comedi seems mostly centered around sync
>> syscalls, where the comedi subdevice stores the file in 'busy' as a
>> marker. This is obviously broken for things like poll which can be
>> persistent across syscalls.
>>
>> Rename is_device_busy() to start_detach(), and move it under the
>> dev->attach_lock. The latter serializes it with poll attempts. If
>> start_detach() is successful, then it will have marked the device with
>> ->detaching == 1 and this will prevent further poll attempts.
>>
>> Similarly, have start_detach() check for active polls on the device, and
>> return busy for that case.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 8ffdff6a8cfb ("staging: comedi: move out of staging directory")
>> Link: https://lore.kernel.org/all/687bd5fe.a70a0220.693ce.0091.GAE@google.com/
>> Reported-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
>> Tested-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
>> index c83fd14dd7ad..58b034e45283 100644
>> --- a/drivers/comedi/comedi_fops.c
>> +++ b/drivers/comedi/comedi_fops.c
>> @@ -782,24 +782,33 @@ void comedi_device_cancel_all(struct comedi_device *dev)
>>       }
>>   }
>>   -static int is_device_busy(struct comedi_device *dev)
>> +static int start_detach(struct comedi_device *dev)
>>   {
>>       struct comedi_subdevice *s;
>> -    int i;
>> +    int i, is_busy = 0;
>>         lockdep_assert_held(&dev->mutex);
>> +    lockdep_assert_held(&dev->attach_lock);
>>       if (!dev->attached)
>>           return 0;
>>         for (i = 0; i < dev->n_subdevices; i++) {
>>           s = &dev->subdevices[i];
>> -        if (s->busy)
>> -            return 1;
>> -        if (s->async && comedi_buf_is_mmapped(s))
>> -            return 1;
>> +        if (s->busy) {
>> +            is_busy = 1;
>> +            break;
>> +        }
>> +        if (!s->async)
>> +            continue;
>> +        if (comedi_buf_is_mmapped(s) ||
>> +            wq_has_sleeper(&s->async->wait_head)) {
>> +            is_busy = 1;
>> +            break;
>> +        }
>>       }
>> -
>> -    return 0;
>> +    if (!is_busy)
>> +        dev->detaching = 1;
>> +    return is_busy;
>>   }
>>     /*
>> @@ -825,8 +834,13 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
>>           return -EPERM;
>>         if (!arg) {
>> -        if (is_device_busy(dev))
>> +        /* prevent new polls */
>> +        down_write(&dev->attach_lock);
>> +        if (start_detach(dev)) {
>> +            up_write(&dev->attach_lock);
>>               return -EBUSY;
>> +        }
>> +        up_write(&dev->attach_lock);
>>           if (dev->attached) {
>>               struct module *driver_module = dev->driver->module;
>>   @@ -2479,7 +2493,7 @@ static __poll_t comedi_poll(struct file *file, poll_table *wait)
>>         down_read(&dev->attach_lock);
>>   -    if (!dev->attached) {
>> +    if (!dev->attached || dev->detaching) {
>>           dev_dbg(dev->class_dev, "no driver attached\n");
>>           goto done;
>>       }
>> diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h
>> index 4cb0400ad616..b2bec668785f 100644
>> --- a/include/linux/comedi/comedidev.h
>> +++ b/include/linux/comedi/comedidev.h
>> @@ -545,6 +545,7 @@ struct comedi_device {
>>       const char *board_name;
>>       const void *board_ptr;
>>       unsigned int attached:1;
>> +    unsigned int detaching:1;
>>       unsigned int ioenabled:1;
>>       spinlock_t spinlock;    /* generic spin-lock for low-level driver */
>>       struct mutex mutex;    /* generic mutex for COMEDI core */
>>
> 
> Thanks for the patch.  I'll post my version shortly.  One problem with
> this patch is that dev->detaching does not get cleared.

I'm fine with your patch, no obligations on this one. Main thing was
getting to the bottom of what that issue was, you know the code better
and can propose a better solution based on that. I only wrote this one
to ensure the issue didn't get ignored.

-- 
Jens Axboe