A race condition may occur if the user physically removes the pcmcia
device while calling open() for this char device node.
This is a race condition between the cmm_open() function and the
cm4000_detach() function, which may eventually result in UAF.
So, add a refcount check to cm4000_detach() to free the "dev" structure
after the char device node is close()d.
Signed-off-by: Hyunwoo Kim <[email protected]>
---
drivers/char/pcmcia/cm4000_cs.c | 58 +++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index adaec8fd4b16..7103812b4019 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -55,6 +55,7 @@
} while (0)
static DEFINE_MUTEX(cmm_mutex);
+static DEFINE_MUTEX(remove_mutex);
#define T_1SEC (HZ)
#define T_10MSEC msecs_to_jiffies(10)
@@ -103,7 +104,8 @@ static int major; /* major number we get from the kernel */
#define REG_STOPBITS(x) (x + 7)
struct cm4000_dev {
- struct pcmcia_device *p_dev;
+ struct pcmcia_device *p_dev;
+ struct kref refcnt;
unsigned char atr[MAX_ATR];
unsigned char rbuf[512];
@@ -146,6 +148,9 @@ struct cm4000_dev {
#define ZERO_DEV(dev) memset(&((dev)->init), 0, sizeof((dev)->init))
+static void stop_monitor(struct cm4000_dev *dev);
+static void cm4000_delete(struct kref *kref);
+
static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
static struct class *cmm_class;
@@ -416,6 +421,30 @@ static struct card_fixup card_fixups[] = {
},
};
+
+static void cm4000_delete(struct kref *kref)
+{
+ struct cm4000_dev *dev = container_of(kref, struct cm4000_dev, refcnt);
+ struct pcmcia_device *link = dev->p_dev;
+ int devno;
+
+ /* find device */
+ for (devno = 0; devno < CM4000_MAX_DEV; devno++)
+ if (dev_table[devno] == link)
+ break;
+ if (devno == CM4000_MAX_DEV)
+ return;
+
+ stop_monitor(dev);
+
+ cm4000_release(link);
+
+ dev_table[devno] = NULL;
+ kfree(dev);
+
+ device_destroy(cmm_class, MKDEV(major, devno));
+}
+
static void set_cardparameter(struct cm4000_dev *dev)
{
int i;
@@ -1629,6 +1658,7 @@ static int cmm_open(struct inode *inode, struct file *filp)
if (minor >= CM4000_MAX_DEV)
return -ENODEV;
+ mutex_lock(&remove_mutex);
mutex_lock(&cmm_mutex);
link = dev_table[minor];
if (link == NULL || !pcmcia_dev_present(link)) {
@@ -1673,8 +1703,12 @@ static int cmm_open(struct inode *inode, struct file *filp)
DEBUGP(2, dev, "<- cmm_open\n");
ret = stream_open(inode, filp);
+
+ kref_get(&dev->refcnt);
out:
mutex_unlock(&cmm_mutex);
+ mutex_unlock(&remove_mutex);
+
return ret;
}
@@ -1703,6 +1737,8 @@ static int cmm_close(struct inode *inode, struct file *filp)
link->open = 0; /* only one open per device */
wake_up(&dev->devq); /* socket removed? */
+ kref_put(&dev->refcnt, cm4000_delete);
+
DEBUGP(2, dev, "cmm_close\n");
return 0;
}
@@ -1808,6 +1844,7 @@ static int cm4000_probe(struct pcmcia_device *link)
init_waitqueue_head(&dev->ioq);
init_waitqueue_head(&dev->atrq);
init_waitqueue_head(&dev->readq);
+ kref_init(&dev->refcnt);
ret = cm4000_config(link, i);
if (ret) {
@@ -1824,23 +1861,10 @@ static int cm4000_probe(struct pcmcia_device *link)
static void cm4000_detach(struct pcmcia_device *link)
{
struct cm4000_dev *dev = link->priv;
- int devno;
-
- /* find device */
- for (devno = 0; devno < CM4000_MAX_DEV; devno++)
- if (dev_table[devno] == link)
- break;
- if (devno == CM4000_MAX_DEV)
- return;
-
- stop_monitor(dev);
-
- cm4000_release(link);
- dev_table[devno] = NULL;
- kfree(dev);
-
- device_destroy(cmm_class, MKDEV(major, devno));
+ mutex_lock(&remove_mutex);
+ kref_put(&dev->refcnt, cm4000_delete);
+ mutex_unlock(&remove_mutex);
return;
}
--
2.25.1
Dear,
I fixed the wrong patch referencing "dev" after kref_put() in the previous version of the patch.
Regards,
Hyunwoo Kim.
Ping -- what's the status of these?
Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?
Thanks.
On 19. 09. 22, 6:07, Hyunwoo Kim wrote:
> A race condition may occur if the user physically removes the pcmcia
> device while calling open() for this char device node.
>
> This is a race condition between the cmm_open() function and the
> cm4000_detach() function, which may eventually result in UAF.
>
> So, add a refcount check to cm4000_detach() to free the "dev" structure
> after the char device node is close()d.
>
> Signed-off-by: Hyunwoo Kim <[email protected]>
> ---
> drivers/char/pcmcia/cm4000_cs.c | 58 +++++++++++++++++++++++----------
> 1 file changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index adaec8fd4b16..7103812b4019 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -55,6 +55,7 @@
> } while (0)
>
> static DEFINE_MUTEX(cmm_mutex);
> +static DEFINE_MUTEX(remove_mutex);
>
> #define T_1SEC (HZ)
> #define T_10MSEC msecs_to_jiffies(10)
> @@ -103,7 +104,8 @@ static int major; /* major number we get from the kernel */
> #define REG_STOPBITS(x) (x + 7)
>
> struct cm4000_dev {
> - struct pcmcia_device *p_dev;
> + struct pcmcia_device *p_dev;
> + struct kref refcnt;
>
> unsigned char atr[MAX_ATR];
> unsigned char rbuf[512];
> @@ -146,6 +148,9 @@ struct cm4000_dev {
>
> #define ZERO_DEV(dev) memset(&((dev)->init), 0, sizeof((dev)->init))
>
> +static void stop_monitor(struct cm4000_dev *dev);
> +static void cm4000_delete(struct kref *kref);
> +
> static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
> static struct class *cmm_class;
>
> @@ -416,6 +421,30 @@ static struct card_fixup card_fixups[] = {
> },
> };
>
> +
> +static void cm4000_delete(struct kref *kref)
> +{
> + struct cm4000_dev *dev = container_of(kref, struct cm4000_dev, refcnt);
> + struct pcmcia_device *link = dev->p_dev;
> + int devno;
> +
> + /* find device */
> + for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> + if (dev_table[devno] == link)
> + break;
> + if (devno == CM4000_MAX_DEV)
> + return;
> +
> + stop_monitor(dev);
> +
> + cm4000_release(link);
> +
> + dev_table[devno] = NULL;
> + kfree(dev);
> +
> + device_destroy(cmm_class, MKDEV(major, devno));
> +}
> +
> static void set_cardparameter(struct cm4000_dev *dev)
> {
> int i;
> @@ -1629,6 +1658,7 @@ static int cmm_open(struct inode *inode, struct file *filp)
> if (minor >= CM4000_MAX_DEV)
> return -ENODEV;
>
> + mutex_lock(&remove_mutex);
> mutex_lock(&cmm_mutex);
> link = dev_table[minor];
> if (link == NULL || !pcmcia_dev_present(link)) {
> @@ -1673,8 +1703,12 @@ static int cmm_open(struct inode *inode, struct file *filp)
>
> DEBUGP(2, dev, "<- cmm_open\n");
> ret = stream_open(inode, filp);
> +
> + kref_get(&dev->refcnt);
> out:
> mutex_unlock(&cmm_mutex);
> + mutex_unlock(&remove_mutex);
> +
> return ret;
> }
>
> @@ -1703,6 +1737,8 @@ static int cmm_close(struct inode *inode, struct file *filp)
> link->open = 0; /* only one open per device */
> wake_up(&dev->devq); /* socket removed? */
>
> + kref_put(&dev->refcnt, cm4000_delete);
> +
> DEBUGP(2, dev, "cmm_close\n");
> return 0;
> }
> @@ -1808,6 +1844,7 @@ static int cm4000_probe(struct pcmcia_device *link)
> init_waitqueue_head(&dev->ioq);
> init_waitqueue_head(&dev->atrq);
> init_waitqueue_head(&dev->readq);
> + kref_init(&dev->refcnt);
>
> ret = cm4000_config(link, i);
> if (ret) {
> @@ -1824,23 +1861,10 @@ static int cm4000_probe(struct pcmcia_device *link)
> static void cm4000_detach(struct pcmcia_device *link)
> {
> struct cm4000_dev *dev = link->priv;
> - int devno;
> -
> - /* find device */
> - for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> - if (dev_table[devno] == link)
> - break;
> - if (devno == CM4000_MAX_DEV)
> - return;
> -
> - stop_monitor(dev);
> -
> - cm4000_release(link);
>
> - dev_table[devno] = NULL;
> - kfree(dev);
> -
> - device_destroy(cmm_class, MKDEV(major, devno));
> + mutex_lock(&remove_mutex);
> + kref_put(&dev->refcnt, cm4000_delete);
> + mutex_unlock(&remove_mutex);
>
> return;
> }
--
js
suse labs
On Tue, Feb 21, 2023, at 07:51, Jiri Slaby wrote:
> Ping -- what's the status of these?
>
> Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?
A few bug fixes ago, I think we had all agreed that the drivers can
just be removed immediately, without a grace period or going through
drivers/staging [1]. We just need someone to send the corresponding
patches.
While looking for those, I see that Dominik also asked the
broader question about PCMCIA drivers in general [2] (sorry
I missed that thread at the time), and Linus just merged my
boardfile removal patches that ended up dropping half of the
(arm32) soc or board specific socket back end drivers.
Among the options that Dominik proposed in that email, I would
prefer we go ahead with b) and remove most of the drivers that
have no known users. I think we can be more aggressive though,
as most of the drivers that are listed as 'some activity in
2020/21/22' seem to only be done to fix the same issues that
were found in ISA or PCI drivers.
The two important use cases that I see for drivers/pcmcia are
cardbus devices on old laptops, which work with normal PCI
device drivers, and CF card storage for embedded systems.
If we can separate the two by moving native cardbus to
drivers/pci/hotplug but drop support for 16-bit PCMCIA
devices in cardbus slots, this will hopefully get a lot
easier.
Arnd
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
On 21. 02. 23, 13:43, Arnd Bergmann wrote:
> On Tue, Feb 21, 2023, at 07:51, Jiri Slaby wrote:
>> Ping -- what's the status of these?
>>
>> Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?
>
> A few bug fixes ago, I think we had all agreed that the drivers can
> just be removed immediately, without a grace period or going through
> drivers/staging [1]. We just need someone to send the corresponding
> patches.
>
> While looking for those, I see that Dominik also asked the
> broader question about PCMCIA drivers in general [2] (sorry
> I missed that thread at the time), and Linus just merged my
> boardfile removal patches that ended up dropping half of the
> (arm32) soc or board specific socket back end drivers.
>
> Among the options that Dominik proposed in that email, I would
> prefer we go ahead with b) and remove most of the drivers that
> have no known users. I think we can be more aggressive though,
> as most of the drivers that are listed as 'some activity in
> 2020/21/22' seem to only be done to fix the same issues that
> were found in ISA or PCI drivers.
So let me start with removal of all (both + and -) listed[2]
drivers/char/pcmcia/ drivers. That includes all three racy/buggy ones.
And let's see what happens.
Personal not: this will also remove synclinc_cs \o/. Mostly only we, the
tty people, were forced to touch the driver and I really hate it.
> [2]
https://lore.kernel.org/all/[email protected]/
thanks,
--
js
suse labs
On Wed, Feb 22, 2023 at 08:53:22AM +0100, Jiri Slaby wrote:
> On 21. 02. 23, 13:43, Arnd Bergmann wrote:
> > On Tue, Feb 21, 2023, at 07:51, Jiri Slaby wrote:
> > > Ping -- what's the status of these?
> > >
> > > Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?
> >
> > A few bug fixes ago, I think we had all agreed that the drivers can
> > just be removed immediately, without a grace period or going through
> > drivers/staging [1]. We just need someone to send the corresponding
> > patches.
> >
> > While looking for those, I see that Dominik also asked the
> > broader question about PCMCIA drivers in general [2] (sorry
> > I missed that thread at the time), and Linus just merged my
> > boardfile removal patches that ended up dropping half of the
> > (arm32) soc or board specific socket back end drivers.
> >
> > Among the options that Dominik proposed in that email, I would
> > prefer we go ahead with b) and remove most of the drivers that
> > have no known users. I think we can be more aggressive though,
> > as most of the drivers that are listed as 'some activity in
> > 2020/21/22' seem to only be done to fix the same issues that
> > were found in ISA or PCI drivers.
>
> So let me start with removal of all (both + and -) listed[2]
> drivers/char/pcmcia/ drivers. That includes all three racy/buggy ones. And
> let's see what happens.
I will GLADLY take these removal patches, please, send them on! :)
thanks,
greg k-h