progress: Check for NULL filename
PATCH 1/1 progress: Check for NULL filename
Avoid a NULL pointer dereference if the upper fs layer hasn't set the file->name field. Files opened through the grub_net_fs interface currently do not have this field set (though perhaps they should?).
--- grub-core/lib/progress.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
grub-core/lib/progress.c +1 -2
@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ;
Signature · 1 line
2.5.0
Quoted 4 lines · dann frazier
Avoid a NULL pointer dereference if the upper fs layer hasn't set the
file->namefield. Files opened through thegrub_net_fsinterface currently do not have this field set (though perhaps they should?).
file->name is set in grub_file_open independently of any filesystem used. How comes it becomes empty? Do you see it in current GIT master?
Quoted 27 lines
Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)grub-core/lib/progress.c +1
-3@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ; --2.5.0
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 10 lines · Andrei Borzenkov
Quoted 4 lines · dann frazier
Avoid a NULL pointer dereference if the upper fs layer hasn't set the
file->namefield. Files opened through thegrub_net_fsinterface currently do not have this field set (though perhaps they should?).
file->nameis set ingrub_file_openindependently of any filesystem used. How comes it becomes empty? Do you see it in current GIT master?
Yeah, I see it with current GIT master. Here's what I believe is happening.
grub_file_open() calls the fs->open callback, *before* it initializes file->name. In the net_fs open callback (haven't checked others), it makes a copy of the file structure and instantiates a bufio file structure for it. It copies the bufio structure over the file structure that was passed in.
Now we return to grub_file_open and set file->name in the (now bufio) file structure. But the original file structure backing the bufio still has a NULL name. When this bufio is read, it calls the read method on this backing file, which causes the progress hook to run and fall over.
Perhaps the fix here is just to make grub_file_open set file->name before the fs_open callback?
grub-core/kern/file.c +3 -1
@@ -99,10 +99,11 @@ grub_file_open (const char *name) goto fail; } + file->name =grub_strdup (name); +if ((file->fs->open) (file, file_name) !=GRUB_ERR_NONE ) goto fail; - file->name =grub_strdup (name); grub_errno =GRUB_ERR_NONE ; for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
@@ -126,6 +127,7 @@ grub_file_open (const char *name)/* if (net) grub_net_close (net); */ +grub_free (file->name);grub_free (file); grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,
Quoted 26 lines
Quoted 26 lines
Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ;_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
14.08.2015 00:04, dann frazier пишет:
Quoted 20 lines · Andrei Borzenkov
Quoted 10 lines
Quoted 4 lines · dann frazier
Avoid a NULL pointer dereference if the upper fs layer hasn't set the
file->namefield. Files opened through thegrub_net_fsinterface currently do not have this field set (though perhaps they should?).
file->nameis set ingrub_file_openindependently of any filesystem used. How comes it becomes empty? Do you see it in current GIT master?Yeah, I see it with current GIT master. Here's what I believe is happening.
grub_file_open()calls thefs->opencallback, *before* it initializesfile->name. In thenet_fsopen callback (haven't checked others), it makes a copy of the file structure and instantiates a bufio file structure for it. It copies the bufio structure over the file structure that was passed in.
Quoted 9 lines
Now we return to
grub_file_openand setfile->namein the (now bufio) file structure. But the original file structure backing the bufio still has a NULL name. When this bufio is read, it calls the read method on this backing file, which causes the progress hook to run and fall over.Perhaps the fix here is just to make
grub_file_opensetfile->namebefore thefs_opencallback?
Then grub_net_fs_open needs to duplicate file->name, otherwise it will result in double free of file->name (first by grub_file_close in grub_bufio_close and then by original grub_file_close).
And grub_file_open ignores NULL return from grub_strdup so we need check in progress anyway.
I wish we could simply use grub_buffile_open, but I do not see easy way.
Could you make combined patch?
Quoted 53 lines
grub-core/kern/file.c +3
-1@@ -99,10 +99,11 @@ grub_file_open (const char *name) goto fail; } + file->name =grub_strdup (name); +if ((file->fs->open) (file, file_name) !=GRUB_ERR_NONE ) goto fail; - file->name =grub_strdup (name); grub_errno =GRUB_ERR_NONE ; for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);@@ -126,6 +127,7 @@ grub_file_open (const char *name)/* if (net) grub_net_close (net); */ +grub_free (file->name);grub_free (file); grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,Quoted 28 lines
Quoted 28 lines
Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ;_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
PATCH 1/1 Avoid NULL pointer dereference in progress module.
grub_net_fs_open() saves off a copy of the file structure it gets passed and uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Since file->name doesn't get set until we return back to grub_file_open(), it means that only the bufio structure gets a valid file->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called on it. Fix this by adding a copy of the filename during the switcheroo. grub_file_close() will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case that grub_strdup() fails when setting file->device->net->name.
Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if grub_strdup() fails in grub_file_open().
--- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-)
grub-core/lib/progress.c +1 -2
@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ;
grub-core/net/net.c +13 -1
@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last =NULL ; file->device->net->name =grub_strdup (name);if (!file->device->net->name) -return grub_errno; + { +grub_free (file); +return grub_errno; + } + file->name =grub_strdup (name); +if (!file->name) + { +grub_free (file->device->net->name); +grub_free (file); +return grub_errno; + } err = file->device->net->protocol->open (file, name);if (err)
@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)grub_netbuff_free (file->device->net->packs.first->nb);grub_net_remove_packet (file->device->net->packs.first); } +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return err;
@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return grub_errno;
Signature · 1 line
2.5.0
Re: [PATCH] Avoid NULL pointer dereference in progress module.
18.08.2015 00:57, dann frazier пишет:
Quoted 7 lines
grub_net_fs_open()saves off a copy of the file structure it gets passed and uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Sincefile->namedoesn't get set until we return back togrub_file_open(), it means that only the bufio structure gets a validfile->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called on it. Fix this by adding a copy of the filename during the switcheroo.
Actually name was already saved off as device->net-name. What about attached patch? I'll commit leak fix separately.
Quoted 62 lines
grub_file_close()will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case thatgrub_strdup()fails when settingfile->device->net->name.Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if
grub_strdup()fails ingrub_file_open().Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name ="" ;grub-core/net/net.c +12
-1@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last =NULL ; file->device->net->name =grub_strdup (name);if (!file->device->net->name) -return grub_errno; + { +grub_free (file); +return grub_errno; + } + file->name =grub_strdup (name); +if (!file->name) + { +grub_free (file->device->net->name); +grub_free (file); +return grub_errno; + } err = file->device->net->protocol->open (file, name);if (err)@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)grub_netbuff_free (file->device->net->packs.first->nb);grub_net_remove_packet (file->device->net->packs.first); } +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return err;
grub-core/net/net.c grub_net_fs_open (struct grub_file *file_out, const char *name) · line 1431
Quoted 7 lines
grub-core/net/net.c +1
-0@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name)grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return grub_errno;
From: Andrei Borzenkov <[email protected]>
Subject: [PATCH] progress: avoid NULL dereference for net files
From original patch by dann frazier <[email protected]>:
grub_net_fs_open() saves off a copy of the file structure it gets passed and uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Since file->name doesn't get set until we return back to grub_file_open(), it means that only the bufio structure gets a valid file->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called on it.
grub_net_fs_open() already saved copy of file name as ->net->name, so change progress module to use it.
Also, grub_file_open may leave file->name as NULL if grub_strdup fails. Check for it.
--- grub-core/lib/progress.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
grub-core/lib/progress.c +7 -1
@@ -23,6 +23,7 @@#include <grub/dl.h> #include <grub/misc.h> #include <grub/normal.h> +#include <grub/net.h> GRUB_MOD_LICENSE ("GPLv3+" );
@@ -70,7 +71,12 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent =grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); +if (file->device->net) + partial_file_name =grub_strrchr (file->device->net->name,'/' ); +else if (file->name)/* grub_file_open() may leave it as NULL */ + partial_file_name =grub_strrchr (file->name,'/' ); +else + partial_file_name =NULL ;if (partial_file_name) partial_file_name++;else
Signature · 1 line
tg: (ba218c1..) u/progress-net-file (depends on: master)
Re: [PATCH] Avoid NULL pointer dereference in progress module.
Quoted 13 lines · Andrei Borzenkov
18.08.2015 00:57, dann frazier пишет:
Quoted 10 lines
grub_net_fs_open()saves off a copy of the file structure it gets passed and
uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Sincefile->namedoesn't get set until we return back togrub_file_open(), it means that only the bufio structure gets a validfile->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called
on it. Fix this by adding a copy of the filename during the switcheroo.
Quoted 2 lines
Actually name was already saved off as device->net-name. What about attached patch? I'll commit leak fix separately.
It does seem like it pokes a hole in the fs abstraction, but it works for me.
-dann
Quoted 76 lines
Quoted 75 lines
grub_file_close()will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case thatgrub_strdup()fails when settingfile->device->net->name.Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if
grub_strdup()fails ingrub_file_open().Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name = "";grub-core/net/net.c +11
-1@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last = NULL; file->device->net->name =grub_strdup (name);if (!file->device->net->name) -return grub_errno; + { +grub_free (file); +return grub_errno; + } + file->name =grub_strdup (name); +if (!file->name) + { +grub_free (file->device->net->name); +grub_free (file); +return grub_errno; + } err = file->device->net->protocol->open (file, name);if (err)
grub-core/net/net.c +1
-0@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_netbuff_free (file->device->net->packs.first->nb);grub_net_remove_packet (file->device->net->packs.first); } +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return err;grub-core/net/net.c +1
-0@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return grub_errno;
Re: [PATCH] Avoid NULL pointer dereference in progress module.
Quoted 14 lines · Dann Frazier
Quoted 13 lines · Andrei Borzenkov
18.08.2015 00:57, dann frazier пишет:
Quoted 10 lines
grub_net_fs_open()saves off a copy of the file structure it gets passed and
uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Sincefile->namedoesn't get set until we return back togrub_file_open(), it means that only the bufio structure gets a validfile->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called
on it. Fix this by adding a copy of the filename during the switcheroo.
Quoted 4 lines
Quoted 2 lines
Actually name was already saved off as device->net-name. What about attached patch? I'll commit leak fix separately.
It does seem like it pokes a hole in the fs abstraction, but it works for me.
hey Andrei,
Did you need anything more from me wrt this fix?
-dann
Quoted 76 lines
Quoted 76 lines
Quoted 75 lines
grub_file_close()will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case thatgrub_strdup()fails when settingfile->device->net->name.Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if
grub_strdup()fails ingrub_file_open().Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name = "";grub-core/net/net.c +11
-1@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last = NULL; file->device->net->name =grub_strdup (name);if (!file->device->net->name) -return grub_errno; + { +grub_free (file); +return grub_errno; + } + file->name =grub_strdup (name); +if (!file->name) + { +grub_free (file->device->net->name); +grub_free (file); +return grub_errno; + } err = file->device->net->protocol->open (file, name);if (err)
grub-core/net/net.c +1
-0@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_netbuff_free (file->device->net->packs.first->nb);grub_net_remove_packet (file->device->net->packs.first); } +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return err;grub-core/net/net.c +1
-0@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return grub_errno;
Re: [PATCH] Avoid NULL pointer dereference in progress module.
21.09.2015 18:11, Dann Frazier пишет:
Quoted 16 lines
Quoted 14 lines · Dann Frazier
Quoted 13 lines · Andrei Borzenkov
18.08.2015 00:57, dann frazier пишет:
Quoted 10 lines
grub_net_fs_open()saves off a copy of the file structure it gets passed and
uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Sincefile->namedoesn't get set until we return back togrub_file_open(), it means that only the bufio structure gets a validfile->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called
on it. Fix this by adding a copy of the filename during the switcheroo.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Actually name was already saved off as device->net-name. What about attached patch? I'll commit leak fix separately.
It does seem like it pokes a hole in the fs abstraction, but it works for me.
hey Andrei,
Did you need anything more from me wrt this fix?
Sorry for delay. We already need to differentiate between disk and net files in other places anyway. So I think it is OK as a simple fix. I committed it.
May be bufio needs to be converted to filter but this is too intrusive for this.
Quoted 78 lines
-dann
Quoted 76 lines
Quoted 76 lines
Quoted 75 lines
grub_file_close()will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case thatgrub_strdup()fails when settingfile->device->net->name.Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if
grub_strdup()fails ingrub_file_open().Signed-off-by dann frazier <[email protected]>--- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-)grub-core/lib/progress.c +1
-2@@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size,0 ); - partial_file_name =grub_strrchr (file->name,'/' ); -if (partial_file_name) +if (file->name && (partial_file_name =grub_strrchr (file->name,'/' ))) partial_file_name++;else partial_file_name = "";grub-core/net/net.c +11
-1@@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last = NULL; file->device->net->name =grub_strdup (name);if (!file->device->net->name) -return grub_errno; + { +grub_free (file); +return grub_errno; + } + file->name =grub_strdup (name); +if (!file->name) + { +grub_free (file->device->net->name); +grub_free (file); +return grub_errno; + } err = file->device->net->protocol->open (file, name);if (err)
grub-core/net/net.c +1
-0@@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_netbuff_free (file->device->net->packs.first->nb);grub_net_remove_packet (file->device->net->packs.first); } +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return err;grub-core/net/net.c +1
-0@@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); +grub_free (file->name);grub_free (file->device->net->name);grub_free (file);return grub_errno;