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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <496a7fa6-0850-41b1-9528-3448a562f25a@kernel.dk>
Date: Tue, 22 Jul 2025 09:36:12 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Ian Abbott <abbotti@....co.uk>, hsweeten@...ionengravers.com,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] comedi: include poll wait state in busy assessment

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@...r.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@...kaller.appspotmail.com
>> Tested-by: syzbot+01523a0ae5600aef5895@...kaller.appspotmail.com
>> Signed-off-by: Jens Axboe <axboe@...nel.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