[PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Seyediman Seyedarab]

During zram_reset_device(), comp_algs[prio] is set to NULL by
zram_destroy_comps() before being reinitialized to the default algorithm.
A concurrent sysfs read can occur between these operations, passing NULL
to strcmp() and causing a crash.

Additionally, there's a use-after-free race where zram_remove() frees the
zram structure while concurrent sysfs operations may still be accessing
it. This is because del_gendisk() doesn't wait for active sysfs operations
to complete - it only removes the files from the filesystem but doesn't
drain active references.

Temporarily add a NULL check in zcomp_available_show() to prevent the
crash. The use-after-free issue requires a more comprehensive fix using
proper reference counting to ensure the zram structure isn't freed while
still in use.

Fixes: e46b8a030d76 ("zram: make compression algorithm selection possible")
Reported-by: syzbot+1a281a451fd8c0945d07@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1a281a451fd8c0945d07
Signed-off-by: Seyediman Seyedarab <ImanDevel@gmail.com>
---
 drivers/block/zram/zcomp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index b1bd1daa0060..98a2a3199ba2 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -95,7 +95,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(backends) - 1; i++) {
-		if (!strcmp(comp, backends[i]->name)) {
+		if (comp && !strcmp(comp, backends[i]->name)) {
 			at += sysfs_emit_at(buf, at, "[%s] ",
 					    backends[i]->name);
 		} else {
-- 
2.50.1

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Sergey Senozhatsky]

On (25/08/03 02:25), Seyediman Seyedarab wrote:
> During zram_reset_device(), comp_algs[prio] is set to NULL by
> zram_destroy_comps() before being reinitialized to the default algorithm.
> A concurrent sysfs read can occur between these operations, passing NULL
> to strcmp() and causing a crash.

zram_reset_device() is called under down_write(&zram->init_lock),
while sysfs reads are called under down_read(&zram->init_lock).
zram_reset_device() doesn't release the write ->init_lock until
default_compressor is set.

I think it may make sense to make sure that show() handlers don't
release the read ->init_lock somewhere in between. I only see one
that does so: recomp_algorithm_show().

---

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 8acad3cc6e6e..ee3aa9cc8595 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1228,13 +1228,7 @@ static void comp_algorithm_set(struct zram *zram, u32 prio, const char *alg)
 static ssize_t __comp_algorithm_show(struct zram *zram, u32 prio,
 				     char *buf, ssize_t at)
 {
-	ssize_t sz;
-
-	down_read(&zram->init_lock);
-	sz = zcomp_available_show(zram->comp_algs[prio], buf, at);
-	up_read(&zram->init_lock);
-
-	return sz;
+	return zcomp_available_show(zram->comp_algs[prio], buf, at);
 }
 
 static int __comp_algorithm_store(struct zram *zram, u32 prio, const char *buf)
@@ -1387,8 +1381,12 @@ static ssize_t comp_algorithm_show(struct device *dev,
 				   char *buf)
 {
 	struct zram *zram = dev_to_zram(dev);
+	ssize_t sz;
 
-	return __comp_algorithm_show(zram, ZRAM_PRIMARY_COMP, buf, 0);
+	down_read(&zram->init_lock);
+	sz = __comp_algorithm_show(zram, ZRAM_PRIMARY_COMP, buf, 0);
+	up_read(&zram->init_lock);
+	return sz;
 }
 
 static ssize_t comp_algorithm_store(struct device *dev,
@@ -1412,6 +1410,7 @@ static ssize_t recomp_algorithm_show(struct device *dev,
 	ssize_t sz = 0;
 	u32 prio;
 
+	down_read(&zram->init_lock);
 	for (prio = ZRAM_SECONDARY_COMP; prio < ZRAM_MAX_COMPS; prio++) {
 		if (!zram->comp_algs[prio])
 			continue;
@@ -1419,7 +1418,7 @@ static ssize_t recomp_algorithm_show(struct device *dev,
 		sz += sysfs_emit_at(buf, sz, "#%d: ", prio);
 		sz += __comp_algorithm_show(zram, prio, buf, sz);
 	}
-
+	up_read(&zram->init_lock);
 	return sz;
 }
 

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Sergey Senozhatsky]

On (25/08/03 02:25), Seyediman Seyedarab wrote:
> Temporarily add a NULL check in zcomp_available_show() to prevent the
> crash. The use-after-free issue requires a more comprehensive fix using
> proper reference counting to ensure the zram structure isn't freed while
> still in use.

Not without a reproducer, sorry.  Per my limited experience, attempts
to fix syzkaller reports w/o reproducers often lead to regressions or
just more problems.

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Seyediman Seyedarab]

On 25/08/05 07:22PM, Sergey Senozhatsky wrote:
> On (25/08/03 02:25), Seyediman Seyedarab wrote:
> > Temporarily add a NULL check in zcomp_available_show() to prevent the
> > crash. The use-after-free issue requires a more comprehensive fix using
> > proper reference counting to ensure the zram structure isn't freed while
> > still in use.
> 
> Not without a reproducer, sorry.  Per my limited experience, attempts
> to fix syzkaller reports w/o reproducers often lead to regressions or
> just more problems.

It can be reproduced with the following code:
#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>

int main()
{
    int hot_remove_fd, comp_alg_fd, disksize_fd;
    char buf[256];
    
    system("modprobe -r zram");
    system("modprobe zram");
    
    disksize_fd = open("/sys/block/zram0/disksize", O_WRONLY);
    if (disksize_fd >= 0) {
        write(disksize_fd, "1073741824", 10);
        close(disksize_fd);
    }

    hot_remove_fd = open("/sys/class/zram-control/hot_remove", O_WRONLY);
    comp_alg_fd = open("/sys/block/zram0/comp_algorithm", O_RDONLY);
    
    write(hot_remove_fd, "0", 1);
    
    for (int i = 0; i < 1000000; i++) {
        lseek(comp_alg_fd, 0, SEEK_SET);
        read(comp_alg_fd, buf, sizeof(buf));
        printf("comp_algorithm: %s", buf);
    }
}

Which produces corrupted output sometimes. (it's a race condition, so it
doesn't happen all the time...)

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Seyediman Seyedarab]

On 25/08/05 09:38AM, Seyediman Seyedarab wrote:
> On 25/08/05 07:22PM, Sergey Senozhatsky wrote:
> > On (25/08/03 02:25), Seyediman Seyedarab wrote:
> > > Temporarily add a NULL check in zcomp_available_show() to prevent the
> > > crash. The use-after-free issue requires a more comprehensive fix using
> > > proper reference counting to ensure the zram structure isn't freed while
> > > still in use.
> > 
> > Not without a reproducer, sorry.  Per my limited experience, attempts
> > to fix syzkaller reports w/o reproducers often lead to regressions or
> > just more problems.
> 
> It can be reproduced with the following code:
> #include <stdlib.h>
> #include <stdio.h>
> #include <fcntl.h>
> #include <unistd.h>
> 
> int main()
> {
>     int hot_remove_fd, comp_alg_fd, disksize_fd;
>     char buf[256];
>     
>     system("modprobe -r zram");
>     system("modprobe zram");
>     
>     disksize_fd = open("/sys/block/zram0/disksize", O_WRONLY);
>     if (disksize_fd >= 0) {
>         write(disksize_fd, "1073741824", 10);
>         close(disksize_fd);
>     }
> 
>     hot_remove_fd = open("/sys/class/zram-control/hot_remove", O_WRONLY);
>     comp_alg_fd = open("/sys/block/zram0/comp_algorithm", O_RDONLY);
>     
>     write(hot_remove_fd, "0", 1);
>     
>     for (int i = 0; i < 1000000; i++) {
>         lseek(comp_alg_fd, 0, SEEK_SET);
>         read(comp_alg_fd, buf, sizeof(buf));
>         printf("comp_algorithm: %s", buf);
>     }
> }
> 
> Which produces corrupted output sometimes. (it's a race condition, so it
> doesn't happen all the time...)

To clarify: the reproducer I provided shows only the use-after-free 
issue where zram structure is freed while sysfs reads are ongoing.

The NULL dereference (which syzbot reported) has a much tighter race 
window: it requires catching the brief moment during zram_reset_device() 
where comp_algs[prio] is NULL between zram_destroy_comps() and 
comp_algorithm_set(). This 'can' be triggered by racing concurrent:
- writes to /sys/block/zram0/reset 
- reads from /sys/block/zram0/comp_algorithm
The window is only a few instructions wide under write lock, so it is 
significantly harder to reproduce than the use-after-free.

Your patch [1] should fixes the NULL deref, but the use-after-free remains.

[1] https://lore.kernel.org/r/20250805101946.1774112-1-senozhatsky@chromium.org

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Seyediman Seyedarab]

On 25/08/05 10:43AM, Seyediman Seyedarab wrote:
> On 25/08/05 09:38AM, Seyediman Seyedarab wrote:
> > On 25/08/05 07:22PM, Sergey Senozhatsky wrote:
> > > On (25/08/03 02:25), Seyediman Seyedarab wrote:
> > > > Temporarily add a NULL check in zcomp_available_show() to prevent the
> > > > crash. The use-after-free issue requires a more comprehensive fix using
> > > > proper reference counting to ensure the zram structure isn't freed while
> > > > still in use.
> > > 
> > > Not without a reproducer, sorry.  Per my limited experience, attempts
> > > to fix syzkaller reports w/o reproducers often lead to regressions or
> > > just more problems.
> > 
> > It can be reproduced with the following code:
> > #include <stdlib.h>
> > #include <stdio.h>
> > #include <fcntl.h>
> > #include <unistd.h>
> > 
> > int main()
> > {
> >     int hot_remove_fd, comp_alg_fd, disksize_fd;
> >     char buf[256];
> >     
> >     system("modprobe -r zram");
> >     system("modprobe zram");
> >     
> >     disksize_fd = open("/sys/block/zram0/disksize", O_WRONLY);
> >     if (disksize_fd >= 0) {
> >         write(disksize_fd, "1073741824", 10);
> >         close(disksize_fd);
> >     }
> > 
> >     hot_remove_fd = open("/sys/class/zram-control/hot_remove", O_WRONLY);
> >     comp_alg_fd = open("/sys/block/zram0/comp_algorithm", O_RDONLY);
> >     
> >     write(hot_remove_fd, "0", 1);
> >     
> >     for (int i = 0; i < 1000000; i++) {
> >         lseek(comp_alg_fd, 0, SEEK_SET);
> >         read(comp_alg_fd, buf, sizeof(buf));
> >         printf("comp_algorithm: %s", buf);
> >     }
> > }
> > 
> > Which produces corrupted output sometimes. (it's a race condition, so it
> > doesn't happen all the time...)
> 
> To clarify: the reproducer I provided shows only the use-after-free 
> issue where zram structure is freed while sysfs reads are ongoing.
> 
> The NULL dereference (which syzbot reported) has a much tighter race 
> window: it requires catching the brief moment during zram_reset_device() 
> where comp_algs[prio] is NULL between zram_destroy_comps() and 
> comp_algorithm_set(). This 'can' be triggered by racing concurrent:
> - writes to /sys/block/zram0/reset 
> - reads from /sys/block/zram0/comp_algorithm
> The window is only a few instructions wide under write lock, so it is 
> significantly harder to reproduce than the use-after-free.
> 
> Your patch [1] should fixes the NULL deref, but the use-after-free remains.
> 
> [1] https://lore.kernel.org/r/20250805101946.1774112-1-senozhatsky@chromium.org

I need to correct my previous statement about the use-after-free issue.

My reproducer was wrong. The garbage output I reported was actually from
an uninitialized buffer in  my test code, not from reading freed memory! 
When the device is removed, the kernel correctly returns -ENODEV
rather than accessing freed memory:

#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>

int main()
{
	int hot_remove_fd, comp_alg_fd, disksize_fd;
	ssize_t nBytes = 0;
	char buf[256] = {0};

	system("modprobe -r zram");
	system("modprobe zram");

	disksize_fd = open("/sys/block/zram0/disksize", O_WRONLY);
	if (disksize_fd >= 0) {
		write(disksize_fd, "1G", 2);
		close(disksize_fd);
	}

	hot_remove_fd = open("/sys/class/zram-control/hot_remove", O_WRONLY);
	comp_alg_fd = open("/sys/block/zram0/comp_algorithm", O_RDONLY);

	write(hot_remove_fd, "0", 1);

	for (int i = 0; i < 1000000; i++) {
		lseek(comp_alg_fd, 0, SEEK_SET);
		nBytes = read(comp_alg_fd, buf, sizeof(buf));
		if (nBytes <= 0) {
			perror("read");
			break;
		}
		printf("comp_alg: %s", buf);
	}
}

Output:
read: No such device

The kernel properly protects against use-after-free in this path. I 
apologize for the confusion.

Kindest Regards,
Seyediman

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Sergey Senozhatsky]

On (25/08/05 18:03), Seyediman Seyedarab wrote:
> I need to correct my previous statement about the use-after-free issue.
> 
> My reproducer was wrong. The garbage output I reported was actually from
> an uninitialized buffer in  my test code, not from reading freed memory! 
> When the device is removed, the kernel correctly returns -ENODEV
> rather than accessing freed memory:

[..]

> The kernel properly protects against use-after-free in this path. I 
> apologize for the confusion.

Great, thanks a lot for the update!

Re: [PATCH] zram: fix NULL pointer dereference in zcomp_available_show()

[Sergey Senozhatsky]

On (25/08/05 10:43), Seyediman Seyedarab wrote:
> The NULL dereference (which syzbot reported) has a much tighter race 
> window: it requires catching the brief moment during zram_reset_device() 
> where comp_algs[prio] is NULL between zram_destroy_comps() and 
> comp_algorithm_set(). This 'can' be triggered by racing concurrent:
> - writes to /sys/block/zram0/reset 
> - reads from /sys/block/zram0/comp_algorithm
> The window is only a few instructions wide under write lock, so it is 
> significantly harder to reproduce than the use-after-free.

I honestly don't understand how this is possible.  comp_algorithm_set()
calls are done under writer ->init_lock, which is mutual exclusive:
neither concurrent writes no concurrent reads can occur in the meantime.