All of lore.kernel.org
 help / color / mirror / Atom feed
From: Glenn Washburn <development@efficientek.com>
To: Daniel Kiper <dkiper@net-space.pl>, grub-devel@gnu.org
Cc: Glenn Washburn <development@efficientek.com>
Subject: [PATCH] cryptodisk: Fix Coverity use after free bug
Date: Sat,  1 Jan 2022 15:48:25 -0600	[thread overview]
Message-ID: <20220101214825.1662409-1-development@efficientek.com> (raw)

The Coverity output is:

  *** CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  /grub-core/disk/cryptodisk.c: 1064 in grub_cryptodisk_scan_device_real()
  1058      cleanup:
  1059       if (askpass)
  1060         {
  1061           cargs->key_len = 0;
  1062           grub_free (cargs->key_data);
  1063         }
  >>>     CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  >>>     Using freed pointer "dev".
  1064       return dev;
  1065     }
  1066
  1067     #ifdef GRUB_UTIL
  1068     #include <grub/util/misc.h>
  1069     grub_err_t

Here the 'dev' variable can point to a freed cryptodisk device if the
function grub_cryptodisk_insert() fails. This can happen only on a OOM
condition, but when this happens grub_cryptodisk_insert() calls grub_free on
the passed device. Since grub_cryptodisk_scan_device_real() assumes that
grub_cryptodisk_insert() is always successful, it will return the device,
though the device was freed.

Change grub_cryptodisk_insert() to not free the passed device on failure.
Then on grub_cryptodisk_insert() failure, free the device pointer. This is
done by going to the label 'error', which will call cryptodisk_close() to
free the device and set the device pointer to NULL, so that a pointer to
freed memory is not returned.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
Having reviewed the Coverity error, I believe this is the fix needed to resolve
the use after free reported by Coverity. However, I do not currently have
Coverity setup, so I don't have a way to test if this is both necessary and
sufficient to resolve the Coverity error. Regardess, I do believe that is does
fix a real use after free bug.

Glenn
---
 grub-core/disk/cryptodisk.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 497097394..e7c4795fd 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -889,10 +889,7 @@ grub_cryptodisk_insert (grub_cryptodisk_t newdev, const char *name,
 {
   newdev->source = grub_strdup (name);
   if (!newdev->source)
-    {
-      grub_free (newdev);
-      return grub_errno;
-    }
+    return grub_errno;
 
   newdev->id = last_cryptodisk_id++;
   newdev->source_id = source->id;
@@ -1044,7 +1041,9 @@ grub_cryptodisk_scan_device_real (const char *name,
     if (ret != GRUB_ERR_NONE)
       goto error;
 
-    grub_cryptodisk_insert (dev, name, source);
+    ret = grub_cryptodisk_insert (dev, name, source);
+    if (ret != GRUB_ERR_NONE)
+      goto error;
 
     goto cleanup;
   }
-- 
2.27.0



             reply	other threads:[~2022-01-01 21:48 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-01 21:48 Glenn Washburn [this message]
2022-01-13 18:44 ` [PATCH] cryptodisk: Fix Coverity use after free bug Daniel Kiper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220101214825.1662409-1-development@efficientek.com \
    --to=development@efficientek.com \
    --cc=dkiper@net-space.pl \
    --cc=grub-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.