From 4a9760afe097fcce1ac11b4f4c902a18905655e4 Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 14:22:37 -0800 Subject: [PATCH 01/12] Incorrect array_size test. ARRAY_SIZE(...) will return `3` for this array with members from 0 to 2, therefore arr[3] is out of bounds. The array length test is off by one and needs fixing. Signed-off-by: Auke Kok --- kmod/src/quorum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/quorum.c b/kmod/src/quorum.c index 7c385448..242804ed 100644 --- a/kmod/src/quorum.c +++ b/kmod/src/quorum.c @@ -1062,7 +1062,7 @@ static char *role_str(int role) [LEADER] = "leader", }; - if (role < 0 || role > ARRAY_SIZE(roles) || !roles[role]) + if (role < 0 || role >= ARRAY_SIZE(roles) || !roles[role]) return "invalid"; return roles[role]; From b25d8e8741624df91490faf38db66cbe6a3fd2ed Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 14:56:52 -0800 Subject: [PATCH 02/12] Plug super leak. We accidentally could leak super here, so make sure to free it. Signed-off-by: Auke Kok --- kmod/src/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/client.c b/kmod/src/client.c index 9706d0b6..cffd2bfa 100644 --- a/kmod/src/client.c +++ b/kmod/src/client.c @@ -435,8 +435,8 @@ static int lookup_mounted_client_item(struct super_block *sb, u64 rid) if (ret == -ENOENT) ret = 0; - kfree(super); out: + kfree(super); return ret; } From 3e6373971119f33c0c7176175442a308e53a43cf Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 14:21:25 -0800 Subject: [PATCH 03/12] plug `df` ioctl leaks. The `type` member and padding are not initialized before being copied to userspace. Signed-off-by: Auke Kok --- kmod/src/ioctl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kmod/src/ioctl.c b/kmod/src/ioctl.c index fea7aae3..432e6d37 100644 --- a/kmod/src/ioctl.c +++ b/kmod/src/ioctl.c @@ -954,6 +954,9 @@ static int copy_alloc_detail_to_user(struct super_block *sb, void *arg, if (args->copied == args->nr) return -EOVERFLOW; + /* .type and .pad need clearing */ + memset(&ade, 0, sizeof(struct scoutfs_ioctl_alloc_detail_entry)); + ade.blocks = blocks; ade.id = id; ade.meta = !!meta; From 021830ab04abbb3785c3510cf1d4b49e4646087c Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 15:48:07 -0800 Subject: [PATCH 04/12] If kzalloc fails, avoid NULL deref. We still assign NULL to sbi->s_fs_info to aid checks in cleanup paths. Signed-off-by: Auke Kok --- kmod/src/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/super.c b/kmod/src/super.c index 16ab2343..3c837160 100644 --- a/kmod/src/super.c +++ b/kmod/src/super.c @@ -512,9 +512,9 @@ static int scoutfs_fill_super(struct super_block *sb, void *data, int silent) sbi = kzalloc(sizeof(struct scoutfs_sb_info), GFP_KERNEL); sb->s_fs_info = sbi; - sbi->sb = sb; if (!sbi) return -ENOMEM; + sbi->sb = sb; ret = assign_random_id(sbi); if (ret < 0) From 4358d57f55ff590960fae9d4b9395355c84fa2ee Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 16:10:58 -0800 Subject: [PATCH 05/12] Avoid possible NULL deref on ENOMEM. Assure that we reschedule even if this happens. Maybe it'll recover. If not, we'll have other issues elsewhere first. Signed-off-by: Auke Kok --- kmod/src/srch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/srch.c b/kmod/src/srch.c index a77c5eff..17f482e2 100644 --- a/kmod/src/srch.c +++ b/kmod/src/srch.c @@ -2301,7 +2301,7 @@ out: scoutfs_inc_counter(sb, srch_compact_error); scoutfs_block_writer_forget_all(sb, &wri); - queue_compact_work(srinf, sc->nr > 0 && ret == 0); + queue_compact_work(srinf, sc != NULL && sc->nr > 0 && ret == 0); kfree(sc); } From 52563d3f736f9a8965382c2a04f42bd9f2c76d5c Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Wed, 5 Mar 2025 15:15:02 -0800 Subject: [PATCH 06/12] Address double copy_to_user, possible 1-byte leak. We shouldn't copy the entire _dirent struct and then copy in the name again right after, just stop at offsetoff(struct, name). Now that we're no longer copying the uninitialized name[3] from ent, there is no more possible 1-byte leak here, too. Signed-off-by: Auke Kok --- kmod/src/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/ioctl.c b/kmod/src/ioctl.c index 432e6d37..a7423362 100644 --- a/kmod/src/ioctl.c +++ b/kmod/src/ioctl.c @@ -1372,7 +1372,7 @@ static long scoutfs_ioc_get_referring_entries(struct file *file, unsigned long a ent.d_type = bref->d_type; ent.name_len = name_len; - if (copy_to_user(uent, &ent, sizeof(struct scoutfs_ioctl_dirent)) || + if (copy_to_user(uent, &ent, offsetof(struct scoutfs_ioctl_dirent, name[0])) || copy_to_user(&uent->name[0], bref->dent.name, name_len) || put_user('\0', &uent->name[name_len])) { ret = -EFAULT; From d2cd610c5317f03242fba6aa47b00833f68d2e8f Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 21 Jul 2025 18:02:40 -0400 Subject: [PATCH 07/12] Fix return of uninit value. The value of `ret` is not initialized. If the writeback list is empty, or, if igrab() fails on the only inode on the list, the value of `ret` is returned without being initialized. This would cause the caller to needlessly have to retry, perhaps possibly make things worse. Signed-off-by: Auke Kok --- kmod/src/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/inode.c b/kmod/src/inode.c index e1ac0e3e..cd6f37bc 100644 --- a/kmod/src/inode.c +++ b/kmod/src/inode.c @@ -2188,7 +2188,7 @@ int scoutfs_inode_walk_writeback(struct super_block *sb, bool write) struct scoutfs_inode_info *si; struct scoutfs_inode_info *tmp; struct inode *inode; - int ret; + int ret = 0; spin_lock(&inf->writeback_lock); From 8c5b09aee8177c312b9243d612b79e9c8f780f23 Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 21 Jul 2025 18:16:54 -0400 Subject: [PATCH 08/12] Prevent masking away inconsistent state in search_sorted_file. In these two error conditions we explicitly set `ret = -EIO` but then `break` to set `ret = 0` immediately again, masking away a critical error code that should be returned. Instead, `goto out` retains the EIO error value for the caller. Signed-off-by: Auke Kok --- kmod/src/srch.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kmod/src/srch.c b/kmod/src/srch.c index 17f482e2..f051d374 100644 --- a/kmod/src/srch.c +++ b/kmod/src/srch.c @@ -856,14 +856,14 @@ static int search_sorted_file(struct super_block *sb, if (pos > SCOUTFS_SRCH_BLOCK_SAFE_BYTES) { /* can only be inconsistency :/ */ ret = -EIO; - break; + goto out; } ret = decode_entry(srb->entries + pos, &sre, &prev); if (ret <= 0) { /* can only be inconsistency :/ */ ret = -EIO; - break; + goto out; } pos += ret; prev = sre; From e704cd7074e7d5c2688cb6682c06c51f869ef881 Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 21 Jul 2025 18:26:40 -0400 Subject: [PATCH 09/12] Fix masking of EIO in compact_logs. Several of the inconsistency error paths already correctly `goto out` but this one has a `break`. This would result in doing a whole lot of work on corrupted data. Make this error path go to `out` instead as the others do. Signed-off-by: Auke Kok --- kmod/src/srch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/srch.c b/kmod/src/srch.c index f051d374..4f08be0c 100644 --- a/kmod/src/srch.c +++ b/kmod/src/srch.c @@ -1865,7 +1865,7 @@ static int compact_logs(struct super_block *sb, if (pos > SCOUTFS_SRCH_BLOCK_SAFE_BYTES) { /* can only be inconsistency :/ */ ret = -EIO; - break; + goto out; } ret = decode_entry(srb->entries + pos, sre, &prev); From 2c4316b09609b12abc2bc485ffd752a9bd023d1b Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 28 Jul 2025 13:28:10 -0400 Subject: [PATCH 10/12] Avoid uninitialized map, flags in ext. This function returns a stack pointer to a struct scoutfs_extent, after setting start, len to an extent found in the proper zone, but it leaves map and flags members unset. Initialize the struct to {0,} avoids passing uninitialized values up the callstack. Signed-off-by: Auke Kok --- kmod/src/alloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/alloc.c b/kmod/src/alloc.c index 40c33c20..0ceaf3b8 100644 --- a/kmod/src/alloc.c +++ b/kmod/src/alloc.c @@ -857,7 +857,7 @@ static int find_zone_extent(struct super_block *sb, struct scoutfs_alloc_root *r .zone = SCOUTFS_FREE_EXTENT_ORDER_ZONE, }; struct scoutfs_extent found; - struct scoutfs_extent ext; + struct scoutfs_extent ext = {0,}; u64 start; u64 len; int nr; From c1e89d597da00d259e51f6451fc8956173a531de Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 28 Jul 2025 13:37:33 -0400 Subject: [PATCH 11/12] Fix NULL dereference on error branch in handle_request. If scoutfs_send_omap_response fails for any reason, req is NULL and we would hit a hard NULL deref during unwinding. Signed-off-by: Auke Kok --- kmod/src/omap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kmod/src/omap.c b/kmod/src/omap.c index c39dbc9c..4731668b 100644 --- a/kmod/src/omap.c +++ b/kmod/src/omap.c @@ -592,7 +592,7 @@ static int handle_request(struct super_block *sb, struct omap_request *req) ret = 0; out: free_rids(&priv_rids); - if (ret < 0) { + if ((ret < 0) && (req != NULL)) { ret = scoutfs_server_send_omap_response(sb, req->client_rid, req->client_id, NULL, ret); free_req(req); From a5dbe7f2861001319cca28115d2b79292812303c Mon Sep 17 00:00:00 2001 From: Auke Kok Date: Mon, 28 Jul 2025 13:45:51 -0400 Subject: [PATCH 12/12] Don't set ret = -ENOMEM and immediately overwrite. It's possible that scoutfs_net_alloc_conn() fails due to -ENOMEM, which is legitimately a failure, thus the code here releases the sock again. But the code block here sets `ret = ENOMEM` and then restarts the loop, which immediately sets `ret = kernel_accept()`, thus overwriting the -ENOMEM error value. We can argue that an ENOMEM error situation here is not catastrophical. If this is the first that we're ever receiving an ENOMEM situation here while trying to accept a new client, we can just release the socket and wait for the client to try again. If the kernel at that point still is out of memory to handle the new incoming connection, that will then cascade down and clean up the while listener at that point. The alternative is to let this error path unwind out and break down the listener immediately, something the code today doesn't do. We're keeping the behavior therefore the same. I've opted therefore to replace the `ret = -ENOMEM` assignment with a comment explaining why we're ignoring the error situation here. Signed-off-by: Auke Kok --- kmod/src/net.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/kmod/src/net.c b/kmod/src/net.c index 380ee637..47a7059f 100644 --- a/kmod/src/net.c +++ b/kmod/src/net.c @@ -1103,9 +1103,15 @@ static void scoutfs_net_listen_worker(struct work_struct *work) conn->notify_down, conn->info_size, conn->req_funcs, "accepted"); + /* + * scoutfs_net_alloc_conn() can fail due to ENOMEM. If this + * is the only thing that does so, there's no harm in trying + * to see if kernel_accept() can get enough memory to try accepting + * a new connection again. If that then fails with ENOMEM, it'll + * shut down the conn anyway. So just retry here. + */ if (!acc_conn) { sock_release(acc_sock); - ret = -ENOMEM; continue; }