Hey there, I spoke with Daniel in April about a bug in the affs file system parser, so I'm trying to post a patch for it here along with the details. Sorry for the delay.
in affs.c, line 412:
```c
/* Create the directory entries for `.' and `..'. */
node = orig_node = grub_zalloc (sizeof (*node));
if (!node)
return 1;
*node = *dir;
if (hook (".", GRUB_FSHELP_DIR, node, hook_data))
return 1;
if (dir->parent)
{
*node = *dir->parent;
if (hook ("..", GRUB_FSHELP_DIR, node, hook_data))
return 1;
}
```
Inside `grub_affs_iterate_dir`, `node` will be freed inside `hook` if the
path does not match ".". If the hook returns 0, then when returning to
`grub_affs_iterate_dir` we eventually reach line 472, where `node` will be
freed again -- at this point `node` and `orig_node` have the same value.
Another potential path for this is entering the function and after
returning from `grub_affs_create_node` it will also be free'd on line 462.
Both of these stem from the fact that both node and orig_node still have
the same value assigned on line 413 which can be freed when going into the
hook logic.
I've attached a reproducer affs filesystem file and a simple grub script to
be run inside grub-emu which demonstrates the bug, all that's needed is to:
```
grub-emu -d <path to grub.cfg dir>
```
You may also have to change the path inside grub.cfg to point to the affs
filesystem file. When done correctly you should observe a double free error.
To mitigate this the attached patch should work but might not be the
optimal solution. Instead of using `node` for the hook logic, we allocate a
separate node which is used. This is very similar to the code in the
`grub_hfsplus_iterate_dir` function:
hfsplus.c, line 851
```c
{
struct grub_fshelp_node *fsnode;
fsnode = grub_malloc (sizeof (*fsnode));
if (!fsnode)
return 1;
*fsnode = *dir;
if (hook (".", GRUB_FSHELP_DIR, fsnode, hook_data))
return 1;
}
```
This area from what I can tell doesnt have this issue.
Please let me know if there are any problems, thanks.
Here's the patch:
diff --git a/grub-core/fs/affs.c b/grub-core/fs/affs.c
index 520a001c7..9e3564b33 100644
--- a/grub-core/fs/affs.c
+++ b/grub-core/fs/affs.c
@@ -414,15 +414,19 @@ grub_affs_iterate_dir (grub_fshelp_node_t dir,
if (!node)
return 1;
- *node = *dir;
- if (hook (".", GRUB_FSHELP_DIR, node, hook_data))
- return 1;
- if (dir->parent)
- {
- *node = *dir->parent;
- if (hook ("..", GRUB_FSHELP_DIR, node, hook_data))
- return 1;
+ {
+ struct grub_fshelp_node *temp_node = grub_zalloc(sizeof(*node));
+ if (!temp_node)
+ return 1;
+ *temp_node = *dir;
+ if (hook(".", GRUB_FSHELP_DIR, temp_node, hook_data))
+ return 1;
+ if (dir->parent) {
+ *temp_node = *dir->parent;
+ if (hook("..", GRUB_FSHELP_DIR, temp_node, hook_data))
+ return 1;
+ }
+ }
tst.affs
Description: Binary data
grub.cfg
Description: Binary data
diff --git a/grub-core/fs/affs.c b/grub-core/fs/affs.c
index 520a001c7..9e3564b33 100644
--- a/grub-core/fs/affs.c
+++ b/grub-core/fs/affs.c
@@ -414,15 +414,19 @@ grub_affs_iterate_dir (grub_fshelp_node_t dir,
if (!node)
return 1;
- *node = *dir;
- if (hook (".", GRUB_FSHELP_DIR, node, hook_data))
- return 1;
- if (dir->parent)
- {
- *node = *dir->parent;
- if (hook ("..", GRUB_FSHELP_DIR, node, hook_data))
- return 1;
+ {
+ struct grub_fshelp_node *temp_node = grub_zalloc(sizeof(*node));
+ if (!temp_node)
+ return 1;
+ *temp_node = *dir;
+ if (hook(".", GRUB_FSHELP_DIR, temp_node, hook_data))
+ return 1;
+ if (dir->parent) {
+ *temp_node = *dir->parent;
+ if (hook("..", GRUB_FSHELP_DIR, temp_node, hook_data))
+ return 1;
}
+ }
hashtable = grub_calloc (data->htsize, sizeof (*hashtable));
if (!hashtable)
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
