Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Andrei Borzenkov <arvidjaar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

18.08.2015 00:57, dann frazier пишет:
> 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.

Actually name was already saved off as device->net-name. What about 
attached patch? I'll commit leak fix separately.

> 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().
>
> Signed-off-by: dann frazier <dann.frazier@canonical.com>
> ---
>   grub-core/lib/progress.c |  3 +--
>   grub-core/net/net.c      | 14 +++++++++++++-
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> index 63a0767..2775554 100644
> --- a/grub-core/lib/progress.c
> +++ b/grub-core/lib/progress.c
> @@ -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 = "";
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 21a4e94..9f83765 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -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;
>


[-- Attachment #2: proress-net.patch --]
[-- Type: text/x-patch, Size: 1906 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] progress: avoid NULL dereference for net files

From original patch by dann frazier <dann.frazier@canonical.com>:

  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.

Also-By: dann frazier <dann.frazier@canonical.com>

---
 grub-core/lib/progress.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 63a0767..7c723eb 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -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
-- 
tg: (ba218c1..) u/progress-net-file (depends on: master)