[PATCH 0/6] fs: ubootvarfs: harden parser

[PATCH 0/6] fs: ubootvarfs: harden parser

[Ahmad Fatoum]

Fix a number of issues in our U-Boot env support, mostly unearthed by
fuzzing with some LLM assistance.

Ahmad Fatoum (6):
  fs: skip directory entries whose name does not fit struct dirent
  fs: reject negative lengths in ftruncate()
  fs: ubootvarfs: range-check the new size in truncate
  fs: ubootvarfs: do not form pointers past the end of the environment
  fs: ubootvarfs: reject variable names containing '='
  fs: ubootvarfs: handle removal of variables that are still open

 fs/fs.c         |  8 ++++++++
 fs/ubootvarfs.c | 36 ++++++++++++++++++++++++++++--------
 2 files changed, 36 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH 1/6] fs: skip directory entries whose name does not fit struct dirent

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

fillonedir() copies whatever name length the file system reports into
the fixed 256 byte d_name of struct dirent. A longer name overruns the
heap allocation and a 256 byte one is left without a terminating NUL.

Such names do exist: ubootvarfs lists U-Boot environment variables,
whose names have no length limit and come from a medium the OS can
rewrite, and ramfs accepts file names of any length. An environment
with a 300 byte variable name corrupts the heap as soon as the
directory is listed.

Let's skip these entries with a warning instead. The rest of the
directory is still listed and the file remains accessible by name.

Fixes: b3fbfad7aeaf ("fs: dentry cache implementation")
Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/fs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index 1426ce4bc883..e18e3b9cd7b2 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1088,6 +1088,11 @@ static int fillonedir(struct dir_context *ctx, const char *name, int namlen,
 	struct readdir_callback *rd = container_of(ctx, struct readdir_callback, ctx);
 	struct readdir_entry *entry;
 
+	if (namlen >= sizeof(entry->d.d_name)) {
+		pr_warn("skipping directory entry %.32s...: name too long\n", name);
+		return 0;
+	}
+
 	entry = xzalloc(sizeof(*entry));
 	memcpy(entry->d.d_name, name, namlen);
 	list_add_tail(&entry->list, &rd->dir->entries);
-- 
2.47.3

[PATCH 2/6] fs: reject negative lengths in ftruncate()

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

ftruncate() passes any length on to the file system driver and then
records it as the new file size. None of the drivers expect a negative
one, so e.g. ubootvarfs moves the rest of the environment to before the
start of its buffer when asked to truncate a variable to -100 bytes.

A negative length is easy to come by: truncate -s parses its argument
as unsigned and truncate -s +SIZE can wrap around. Let's return -EINVAL
for it like POSIX says, before any driver sees it.

Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/fs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index e18e3b9cd7b2..977635d41082 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -393,6 +393,9 @@ int ftruncate(int fd, loff_t length)
 	if (IS_ERR(f))
 		return -errno;
 
+	if (length < 0)
+		return errno_set(-EINVAL);
+
 	if (!i_size_is_bound(f->f_inode))
 		return 0;
 
-- 
2.47.3

[PATCH 3/6] fs: ubootvarfs: range-check the new size in truncate

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

ubootvarfs_truncate() stores the size difference in an int and checks
the room left by forming a pointer that far past the end of the
environment. A size 4 GiB past the current one wraps to a difference of
zero, the truncate succeeds and the VFS records a 4 GiB file size. As
ubootvarfs_io() relies on the VFS to clamp reads and writes to that
size, md and mw on the variable then go well past the environment
buffer. A size 2 GiB past the current one wraps to a large negative
difference and the tail is moved 2 GiB below the buffer.

Let's keep the difference in a loff_t, compare it against the room left
instead of forming the pointer and reject negative sizes, which the
driver cannot handle either. The resize helpers take a ptrdiff_t now,
so the checked difference reaches them unchanged.

Fixes: 8daaa21b3949 ("fs: Add a driver to access U-Boot environment variables")
Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/ubootvarfs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
index a703f16a10f3..3b5c3039e3aa 100644
--- a/fs/ubootvarfs.c
+++ b/fs/ubootvarfs.c
@@ -197,7 +197,7 @@ static const struct file_operations ubootvarfs_dir_operations = {
  * zeroed out
  */
 static void ubootvarfs_relocate_tail(struct ubootvarfs_inode *node,
-				     int delta)
+				     ptrdiff_t delta)
 {
 	struct ubootvarfs_var *var = node->var;
 	struct ubootvarfs_data *data = node->data;
@@ -235,7 +235,7 @@ static void ubootvarfs_relocate_tail(struct ubootvarfs_inode *node,
  * ubootvarfs_var's in varaible linked list
  */
 static void ubootvarfs_adjust(struct ubootvarfs_inode *node,
-			      int delta)
+			      ptrdiff_t delta)
 {
 	struct ubootvarfs_var *var = node->var;
 	struct ubootvarfs_data *data = node->data;
@@ -260,7 +260,7 @@ static int ubootvarfs_unlink(struct inode *dir, struct dentry *dentry)
 		 * -1 at the end is to account for '\0' at the end
 		 * that needs to be removed as well
 		 */
-		const int delta = var->name - var->end - 1;
+		const ptrdiff_t delta = var->name - var->end - 1;
 
 		ubootvarfs_adjust(node, delta);
 
@@ -369,12 +369,16 @@ static int ubootvarfs_truncate(struct file *f, loff_t size)
 	struct ubootvarfs_inode *node = inode_to_node(inode);
 	struct ubootvarfs_data *data = node->data;
 	struct ubootvarfs_var *var = node->var;
-	const int delta = size - inode->i_size;
+	loff_t delta;
+
+	if (size < 0)
+		return -EINVAL;
 
 	if (size == inode->i_size)
 		return 0;
 
-	if (data->end + delta >= data->limit)
+	delta = size - inode->i_size;
+	if (delta >= data->limit - data->end)
 		return -ENOSPC;
 
 	ubootvarfs_adjust(node, delta);
-- 
2.47.3

[PATCH 4/6] fs: ubootvarfs: do not form pointers past the end of the environment

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

When the environment fills its whole partition, data->end is one past
the end of the mapping. The room check in create adds the length of
the new name to it and tail relocation adds one, both yielding pointers
further past the end, which C leaves undefined even if they are only
compared.

Let's compare the room left against the length instead and only step
past data->end when it is still inside the mapping. No change in
behavior with the compilers we use.

Fixes: 8daaa21b3949 ("fs: Add a driver to access U-Boot environment variables")
Fixes: 79f048173d98 ("fs: ubootvarfs: fix tail relocation length computation")
Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/ubootvarfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
index 3b5c3039e3aa..72d0627fb0cd 100644
--- a/fs/ubootvarfs.c
+++ b/fs/ubootvarfs.c
@@ -210,7 +210,7 @@ static void ubootvarfs_relocate_tail(struct ubootvarfs_inode *node,
 	 * the last entry's NUL is the final byte and data->end points
 	 * one past the mapping, so there is no terminator to carry along.
 	 */
-	tail_end = min_t(const char *, data->end + 1, data->limit);
+	tail_end = data->end == data->limit ? data->end : data->end + 1;
 
 	memmove(src + delta, src, tail_end - src);
 
@@ -286,7 +286,7 @@ static int ubootvarfs_create(struct inode *dir, struct dentry *dentry,
 	 * we need to make sure there's enough room for it. Note that
 	 * + 3 is to accoutn for '=', and two '\0' from above
 	 */
-	if (data->end + len + 3 > data->limit)
+	if (len + 3 > data->limit - data->end)
 		return -ENOSPC;
 
 	var = xmalloc(sizeof(*var));
-- 
2.47.3

[PATCH 5/6] fs: ubootvarfs: reject variable names containing '='

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

Creating a file named x=y on ubootvarfs stores the variable as
"x=y=<value>". That works until the environment is parsed again, which
splits it at the first '=' into a variable x with the value
"y=<value>". The file the user created is gone after a remount or
reboot and another one has taken its place.

U-Boot cannot represent such a name either, so let's refuse to create
it with -EINVAL.

Fixes: 8daaa21b3949 ("fs: Add a driver to access U-Boot environment variables")
Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/ubootvarfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
index 72d0627fb0cd..b0353d7cedef 100644
--- a/fs/ubootvarfs.c
+++ b/fs/ubootvarfs.c
@@ -281,6 +281,11 @@ static int ubootvarfs_create(struct inode *dir, struct dentry *dentry,
 	struct inode *inode;
 	struct ubootvarfs_var *var;
 	size_t len = strlen(dentry->name);
+
+	/* U-Boot splits a variable at its first '=' */
+	if (strchr(dentry->name, '='))
+		return -EINVAL;
+
 	/*
 	 * We'll be adding <varname>=\0\0 to the end of our data, so
 	 * we need to make sure there's enough room for it. Note that
-- 
2.47.3

[PATCH 6/6] fs: ubootvarfs: handle removal of variables that are still open

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

Removing a variable frees it and clears the inode's pointer to it, but
an open file keeps the inode around with the old size. Reading, writing
or truncating it then dereferences the NULL pointer. The shell can get
there with a loop mount: after mount -o loop on a variable and rm of
it, md on /dev/loop0 or reading a file from the mount crashes.

Let's set the size of a removed variable to zero and return -ENOENT
for any further access to it.

Fixes: 8daaa21b3949 ("fs: Add a driver to access U-Boot environment variables")
Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 fs/ubootvarfs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
index b0353d7cedef..e2d2a2f4aa77 100644
--- a/fs/ubootvarfs.c
+++ b/fs/ubootvarfs.c
@@ -266,7 +266,9 @@ static int ubootvarfs_unlink(struct inode *dir, struct dentry *dentry)
 
 		list_del(&var->list);
 		free(var);
+		/* open files keep the inode, see ubootvarfs_io() */
 		node->var = NULL;
+		inode->i_size = 0;
 	}
 
 	return simple_unlink(dir, dentry);
@@ -348,7 +350,13 @@ static int ubootvarfs_io(struct file *f, void *buf, size_t insize, bool read)
 {
 	struct inode *inode = f->f_inode;
 	struct ubootvarfs_inode *node = inode_to_node(inode);
-	void *ptr = node->var->start + f->f_pos;
+	void *ptr;
+
+	/* the variable was removed while the file was open */
+	if (!node->var)
+		return -ENOENT;
+
+	ptr = node->var->start + f->f_pos;
 
 	if (read)
 		memcpy(buf, ptr, insize);
@@ -376,6 +384,9 @@ static int ubootvarfs_truncate(struct file *f, loff_t size)
 	struct ubootvarfs_var *var = node->var;
 	loff_t delta;
 
+	if (!var)
+		return -ENOENT;
+
 	if (size < 0)
 		return -EINVAL;
 
-- 
2.47.3