thr3ads.net - Libguestfs - [Libguestfs] [PATCH v3] ldmtool: fix NULL pointer dereference [Jun 2023]

If this information is useful, please help other people find it:
Share via:

Vincent Mailhol

2023-Jun-19 14:36 UTC

[Libguestfs] [PATCH v1] ldmtool: fix NULL pointer dereference

If /sys/block can not be opened, get_devices() returns NULL.

cmdline() does not check this result and below code snippet:

          scanned = get_devices();
          devices = (gchar **) scanned->data;

results in a segmentation fault.

Add a check on scanned.

Relevant logs:

  Unable to open /sys/block: No such file or directory
  [    0.777352] ldmtool[164]: segfault at 0 ip 0000563a225cd6a5 sp
00007ffe54965a60 error 4 in ldmtool[563a225cb000+3000]
  [    0.778278] Code: 18 64 48 33 1c 25 28 00 00 00 75 5e 48 83 c4 28 5b 5d 41
5c 41 5d 41 5e 41 5f c3 66 2e 0f 1f 84 00 00 00 00 00 e8 db fd ff ff <4c>
8b 20 48 89 44 24 08 4c 89 e7 e8 0b e1 ff ff 45 31 c0 4c 89 e1

Fixes: 25d9635e4ee5 ("Add ldmtool")
Signed-off-by: Vincent Mailhol <mailhol.vincent at wanadoo.fr>
---
This thread did not yet show-up in
  https://listman.redhat.com/archives/libguestfs/2023-June/subject.html
not sure why.

For this reason, I couln't add a link reference.
---
 src/ldmtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/ldmtool.c b/src/ldmtool.c
index 6957c1a..87aaccc 100644
--- a/src/ldmtool.c
+++ b/src/ldmtool.c
@@ -746,6 +746,8 @@ cmdline(LDM * const ldm, gchar **devices,
     GArray * scanned = NULL;
     if (!devices) {
         scanned = get_devices();
+        if (!scanned)
+            goto error;
         devices = (gchar **) scanned->data;
     }
 
-- 
2.25.1

Laszlo Ersek

2023-Jun-19 17:28 UTC

head link

[Libguestfs] [PATCH v1] ldmtool: fix NULL pointer dereference

On 6/19/23 16:36, Vincent Mailhol wrote:> If /sys/block can not be opened, get_devices() returns NULL.
> 
> cmdline() does not check this result and below code snippet:
> 
>           scanned = get_devices();
>           devices = (gchar **) scanned->data;
> 
> results in a segmentation fault.
> 
> Add a check on scanned.
> 
> Relevant logs:
> 
>   Unable to open /sys/block: No such file or directory
>   [    0.777352] ldmtool[164]: segfault at 0 ip 0000563a225cd6a5 sp
00007ffe54965a60 error 4 in ldmtool[563a225cb000+3000]
>   [    0.778278] Code: 18 64 48 33 1c 25 28 00 00 00 75 5e 48 83 c4 28 5b
5d 41 5c 41 5d 41 5e 41 5f c3 66 2e 0f 1f 84 00 00 00 00 00 e8 db fd ff ff
<4c> 8b 20 48 89 44 24 08 4c 89 e7 e8 0b e1 ff ff 45 31 c0 4c 89 e1
> 
> Fixes: 25d9635e4ee5 ("Add ldmtool")
> Signed-off-by: Vincent Mailhol <mailhol.vincent at wanadoo.fr>
> ---
> This thread did not yet show-up in
>   https://listman.redhat.com/archives/libguestfs/2023-June/subject.html
> not sure why.
> 
> For this reason, I couln't add a link reference.
> ---
>  src/ldmtool.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/ldmtool.c b/src/ldmtool.c
> index 6957c1a..87aaccc 100644
> --- a/src/ldmtool.c
> +++ b/src/ldmtool.c
> @@ -746,6 +746,8 @@ cmdline(LDM * const ldm, gchar **devices,
>      GArray * scanned = NULL;
>      if (!devices) {
>          scanned = get_devices();
> +        if (!scanned)
> +            goto error;
>          devices = (gchar **) scanned->data;
>      }
>  
(I'm reviewing this against commit 5014da5b9071, in repository
<https://github.com/mdbooth/libldm.git>.)

This fix is almost right, but not entirely. Consider what we have under
the "error" label:

error:
    if (scanned) g_array_unref(scanned);
    if (jb) g_object_unref(jb);
    return FALSE;

When we jump to "error" on the new branch, "scanned" is is
NULL, so that
is safe. However, the

    JsonBuilder *jb = NULL;

line has not been reached yet, and that's a problem, because
g_object_unref() will receive an indeterminate value (undefined
behavior). (In fact, just *evaluating* "jb" as the controlling
expression of the "if" that is supposed to protect g_object_unref() is
*already* undefined behavior.)

According to C99 6.8 "Statements and blocks" paragraph 3,

    A /block/ allows a set of declarations and statements to be grouped
    into one syntactic unit. The initializers of objects that have
    automatic storage duration [...] are evaluated and the values are
    stored in the objects (including storing an indeterminate value in
    objects without an initializer) each time the declaration is reached
    in the order of execution, as if it were a statement, and within
    each declaration in the order that declarators appear.

Furthermore, C99 6.8.4.2 "The switch statement" paragraph 7 provides
the
following example:

    In the artificial program fragment

    switch (expr)
    {
        int i = 4;
        f(i);
    case 0:
        i = 17;
        /* falls through into default code */
    default:
        printf("%d\n", i);
    }

    the object whose identifier is *i* exists with automatic storage
    duration (within the block) but is never initialized, and thus if
    the controlling expression has a nonzero value, the call to the
    *printf* function will access an indeterminate value. Similarly, the
    call to the function *f* cannot be reached.

Thus, you can do one of two things (at least):

- *In addition* to the current contents of the patch, move the
declaration of "jb", together with its NULL initializer, to the top of
the function (just below that of "scanned"). This makes sure that the
new jump to "error" will still "pass" a well-defined
"jb" value (NULL),
which the logic at the "error" label handles well.

- Alternatively: replace "goto error" with just "return
FALSE" in your
patch. At that point, there is nothing to release yet.

Laszlo

Vincent Mailhol

2023-Jun-20 07:56 UTC

head link

[Libguestfs] [PATCH v2] ldmtool: fix NULL pointer dereference

From: Vincent Mailhol <vincent.mailhol at woven.toyota>

If /sys/block can not be opened, get_devices() returns NULL.

cmdline() does not check this result and below code snippet:

          scanned = get_devices();
          devices = (gchar **) scanned->data;

results in a segmentation fault.

Add a check on scanned.

Relevant logs:

  Unable to open /sys/block: No such file or directory
  [    0.777352] ldmtool[164]: segfault at 0 ip 0000563a225cd6a5 sp
00007ffe54965a60 error 4 in ldmtool[563a225cb000+3000]
  [    0.778278] Code: 18 64 48 33 1c 25 28 00 00 00 75 5e 48 83 c4 28 5b 5d 41
5c 41 5d 41 5e 41 5f c3 66 2e 0f 1f 84 00 00 00 00 00 e8 db fd ff ff <4c>
8b 20 48 89 44 24 08 4c 89 e7 e8 0b e1 ff ff 45 31 c0 4c 89 e1

Fixes: 25d9635e4ee5 ("Add ldmtool")
Signed-off-by: Vincent Mailhol <mailhol.vincent at wanadoo.fr>
---

* Changelog *

v1 -> v2

  * Directly return FALSE instead of goto error. Jumping to the error
    label bypasses jb's declaration thus resulting in an undefined
    behavior.

---
 src/ldmtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/ldmtool.c b/src/ldmtool.c
index 6957c1a..dbe2c8c 100644
--- a/src/ldmtool.c
+++ b/src/ldmtool.c
@@ -746,6 +746,8 @@ cmdline(LDM * const ldm, gchar **devices,
     GArray * scanned = NULL;
     if (!devices) {
         scanned = get_devices();
+        if (!scanned)
+            return FALSE;
         devices = (gchar **) scanned->data;
     }
 
-- 
2.25.1

Vincent Mailhol

2023-Jun-20 08:00 UTC

head link

[Libguestfs] [PATCH v3] ldmtool: fix NULL pointer dereference

If /sys/block can not be opened, get_devices() returns NULL.

cmdline() does not check this result and below code snippet:

          scanned = get_devices();
          devices = (gchar **) scanned->data;

results in a segmentation fault.

Add a check on scanned.

Relevant logs:

  Unable to open /sys/block: No such file or directory
  [    0.777352] ldmtool[164]: segfault at 0 ip 0000563a225cd6a5 sp
00007ffe54965a60 error 4 in ldmtool[563a225cb000+3000]
  [    0.778278] Code: 18 64 48 33 1c 25 28 00 00 00 75 5e 48 83 c4 28 5b 5d 41
5c 41 5d 41 5e 41 5f c3 66 2e 0f 1f 84 00 00 00 00 00 e8 db fd ff ff <4c>
8b 20 48 89 44 24 08 4c 89 e7 e8 0b e1 ff ff 45 31 c0 4c 89 e1

Fixes: 25d9635e4ee5 ("Add ldmtool")
Signed-off-by: Vincent Mailhol <mailhol.vincent at wanadoo.fr>
---

* Changelog *

v2 -> v3

  * Fix the From: tag (incorrect e-mail address, sorry for the noise).

v1 -> v2

  * Directly return FALSE instead of goto error. Jumping to the error
    label bypasses jb's declaration thus resulting in an undefined
    behavior.

---
 src/ldmtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/ldmtool.c b/src/ldmtool.c
index 6957c1a..dbe2c8c 100644
--- a/src/ldmtool.c
+++ b/src/ldmtool.c
@@ -746,6 +746,8 @@ cmdline(LDM * const ldm, gchar **devices,
     GArray * scanned = NULL;
     if (!devices) {
         scanned = get_devices();
+        if (!scanned)
+            return FALSE;
         devices = (gchar **) scanned->data;
     }
 
-- 
2.25.1

Libguestfs - Jun 2023 - [PATCH v3] ldmtool: fix NULL pointer dereference

[Libguestfs] [PATCH v1] ldmtool: fix NULL pointer dereference

[Libguestfs] [PATCH v1] ldmtool: fix NULL pointer dereference

[Libguestfs] [PATCH v2] ldmtool: fix NULL pointer dereference

[Libguestfs] [PATCH v3] ldmtool: fix NULL pointer dereference