* [PATCH] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
@ 2023-02-25 11:43 soeren
2023-02-25 12:41 ` [PATCH v2] " soeren
0 siblings, 1 reply; 6+ messages in thread
From: soeren @ 2023-02-25 11:43 UTC (permalink / raw
To: util-linux
From: Sören Tempel <soeren@soeren-tempel.net>
Consider the following libmount example program:
#include <libmount.h>
int
main(void)
{
mnt_match_options("", "+");
return 0;
}
Compiling this program and executing it with valgrind(1) will yield
the following warning regarding a conditional jump depending on an
uninitialised value:
Conditional jump or move depends on uninitialised value(s)
at 0x48AA61B: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x48C6154: ??? (in /lib/libmount.so.1.1.0)
by 0x48C65A0: mnt_optstr_get_option (in /lib/libmount.so.1.1.0)
by 0x48C7B85: mnt_match_options (in /lib/libmount.so.1.1.0)
by 0x1091C1: main (util-linux-test.c:6)
This is because if name == "+" then we advance to the null byte
in name due to the following code in mnt_match_options():
if (*name == '+')
name++, namesz--;
This will cause the `xstrncpy(buf, name, namesz + 1)` invocation in
mnt_match_options() to copy nothing to the destination buffer. The
buffer (buf) is therefore passed uninitialized as the name argument
to mnt_optstr_get_option(). When mnt_optstr_locate_option() (which
is called by mnt_optstr_get_option) attempts to determine the
length of the name argument using strlen(3) then everything blows
up because the name argument is not initialized.
This patch fixes this issue by initializing the buf argument in
mnt_match_options() with NULL before calling xstrncpy thereby
ensuring that buf is /always/ initialized even if xstrncpy
returns without copying any data to the destination buffer
due to the following early return in xstrncpy:
size_t len = src ? strlen(src) : 0;
if (!len)
return;
This issue has been discovered using KLEE <https://klee.github.io/>.
Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
---
libmount/src/optstr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index a8b56e212..ae3efc78b 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -853,6 +853,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
else if ((no = (startswith(name, "no") != NULL)))
name += 2, namesz -= 2;
+ buf = NULL; /* ensure buf is initialized even if name == "" */
xstrncpy(buf, name, namesz + 1);
rc = mnt_optstr_get_option(optstr, buf, &val, &sz);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
2023-02-25 11:43 [PATCH] libmount: Fix access to uninitialised value in mnt_optstr_locate_option soeren
@ 2023-02-25 12:41 ` soeren
2023-02-25 13:40 ` Sören Tempel
0 siblings, 1 reply; 6+ messages in thread
From: soeren @ 2023-02-25 12:41 UTC (permalink / raw
To: util-linux
From: Sören Tempel <soeren@soeren-tempel.net>
Consider the following libmount example program:
#include <libmount.h>
int
main(void)
{
mnt_match_options("", "+");
return 0;
}
Compiling this program and executing it with valgrind(1) will yield
the following warning regarding a conditional jump depending on an
uninitialised value:
Conditional jump or move depends on uninitialised value(s)
at 0x48AA61B: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x48C6154: ??? (in /lib/libmount.so.1.1.0)
by 0x48C65A0: mnt_optstr_get_option (in /lib/libmount.so.1.1.0)
by 0x48C7B85: mnt_match_options (in /lib/libmount.so.1.1.0)
by 0x1091C1: main (util-linux-test.c:6)
This is because if name == "+" then we advance to the null byte
in name due to the following code in mnt_match_options():
if (*name == '+')
name++, namesz--;
This will cause the `xstrncpy(buf, name, namesz + 1)` invocation in
mnt_match_options() to copy nothing to the destination buffer. The
buffer (buf) is therefore passed uninitialized as the name argument
to mnt_optstr_get_option(). When mnt_optstr_locate_option() (which
is called by mnt_optstr_get_option) attempts to determine the
length of the name argument using strlen(3) then everything blows
up because the name argument is not initialized.
This patch fixes this issue by initializing the buf argument in
mnt_match_options() with NULL before calling xstrncpy thereby
ensuring that buf is /always/ initialized even if xstrncpy
returns without copying any data to the destination buffer
due to the following early return in xstrncpy:
size_t len = src ? strlen(src) : 0;
if (!len)
return;
This issue has been discovered using KLEE <https://klee.github.io/>.
Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
---
Changes since v1: Obviously, we shouldn't set buf to NULL
unconditionally. Instead, duplicate the check for the xstrncpy early
return in mnt_match_options and thereby only set buf to NULL if xstrncpy
didn't copy data to it. While at it, also free buf. This is a bit hacky,
ideally xstrncpy should be modified in a way that it never leaves the
first argument uninitialized I suppose?
libmount/src/optstr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index a8b56e212..719e16ab3 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -854,6 +854,10 @@ int mnt_match_options(const char *optstr, const char *pattern)
name += 2, namesz -= 2;
xstrncpy(buf, name, namesz + 1);
+ if (namesz == 0) { /* if xstrncpy didn't copy anything */
+ free(buf);
+ buf = NULL;
+ }
rc = mnt_optstr_get_option(optstr, buf, &val, &sz);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
2023-02-25 12:41 ` [PATCH v2] " soeren
@ 2023-02-25 13:40 ` Sören Tempel
2023-02-27 10:50 ` Karel Zak
0 siblings, 1 reply; 6+ messages in thread
From: Sören Tempel @ 2023-02-25 13:40 UTC (permalink / raw
To: util-linux
soeren@soeren-tempel.net wrote:
> xstrncpy(buf, name, namesz + 1);
> + if (namesz == 0) { /* if xstrncpy didn't copy anything */
> + free(buf);
> + buf = NULL;
> + }
This solution also has the issue that it handles the case incorrectly
where xstrncpy actually calls strlen(3). I wanted to avoid that but
maybe the best solution is to just change the xstrncpy API in a way that
it indicates whether it has written anything through an int return
value?
Sorry for all the noise. Next time, I will just write a bug report...
Greetings,
Sören
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
2023-02-25 13:40 ` Sören Tempel
@ 2023-02-27 10:50 ` Karel Zak
2023-02-27 19:00 ` Sören Tempel
0 siblings, 1 reply; 6+ messages in thread
From: Karel Zak @ 2023-02-27 10:50 UTC (permalink / raw
To: Sören Tempel; +Cc: util-linux
On Sat, Feb 25, 2023 at 02:40:52PM +0100, Sören Tempel wrote:
> This solution also has the issue that it handles the case incorrectly
> where xstrncpy actually calls strlen(3). I wanted to avoid that but
> maybe the best solution is to just change the xstrncpy API in a way that
> it indicates whether it has written anything through an int return
> value?
The best will be to avoid all the buffer, xstrndup(), and
mnt_optstr_get_option() using if the options string or the pattern is
empty. It's unnecessary because we know that mnt_optstr_get_option()
will return nothing.
I have committed
https://github.com/util-linux/util-linux/commit/06ee5267516761721ebfbdfa313980cef8e54c66
> Sorry for all the noise. Next time, I will just write a bug report...
That's no problem. We should be friendly to people who invest time to
report bugs and write code ;-)
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
2023-02-27 10:50 ` Karel Zak
@ 2023-02-27 19:00 ` Sören Tempel
2023-02-28 11:02 ` Karel Zak
0 siblings, 1 reply; 6+ messages in thread
From: Sören Tempel @ 2023-02-27 19:00 UTC (permalink / raw
To: Karel Zak; +Cc: util-linux
Hey Karel,
Thanks for your patch, the changes look good to me!
I have one small follow-up question though. You added the following
sentence to the documentation for mnt_match_options():
> The alone "no" is error and all matching ends with False.
I believe this to be a good change. However, If my understanding of
this documentation comment is correct I would have expected
mnt_match_options("bla", "no,,")
to return False as well but to my surprise it actually returns True. I
understand that this is an edge case but is that intended? Maybe the ","
should be treated as an terminating character for the "no" as well (i.e.
in addition to the null byte)?
diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index 4fbbb0859..a0056c767 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -865,7 +865,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
name++, namesz--;
else if ((no = (startswith(name, "no") != NULL))) {
name += 2, namesz -= 2;
- if (!*name) {
+ if (!*name || *name == ',') {
match = 0;
break; /* alone "no" keyword is error */
}
With this patch applied it also returns False for the "no,," pattern.
Greetings,
Sören
Karel Zak <kzak@redhat.com> wrote:
> On Sat, Feb 25, 2023 at 02:40:52PM +0100, Sören Tempel wrote:
> > This solution also has the issue that it handles the case incorrectly
> > where xstrncpy actually calls strlen(3). I wanted to avoid that but
> > maybe the best solution is to just change the xstrncpy API in a way that
> > it indicates whether it has written anything through an int return
> > value?
>
> The best will be to avoid all the buffer, xstrndup(), and
> mnt_optstr_get_option() using if the options string or the pattern is
> empty. It's unnecessary because we know that mnt_optstr_get_option()
> will return nothing.
>
> I have committed
> https://github.com/util-linux/util-linux/commit/06ee5267516761721ebfbdfa313980cef8e54c66
>
> > Sorry for all the noise. Next time, I will just write a bug report...
>
> That's no problem. We should be friendly to people who invest time to
> report bugs and write code ;-)
>
> Karel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option
2023-02-27 19:00 ` Sören Tempel
@ 2023-02-28 11:02 ` Karel Zak
0 siblings, 0 replies; 6+ messages in thread
From: Karel Zak @ 2023-02-28 11:02 UTC (permalink / raw
To: Sören Tempel; +Cc: util-linux
On Mon, Feb 27, 2023 at 08:00:55PM +0100, Sören Tempel wrote:
> Thanks for your patch, the changes look good to me!
Thanks.
> I have one small follow-up question though. You added the following
> sentence to the documentation for mnt_match_options():
>
> > The alone "no" is error and all matching ends with False.
>
> I believe this to be a good change. However, If my understanding of
> this documentation comment is correct I would have expected
>
> mnt_match_options("bla", "no,,")
Good point.
>
> to return False as well but to my surprise it actually returns True. I
> understand that this is an edge case but is that intended? Maybe the ","
> should be treated as an terminating character for the "no" as well (i.e.
> in addition to the null byte)?
>
> diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
> index 4fbbb0859..a0056c767 100644
> --- a/libmount/src/optstr.c
> +++ b/libmount/src/optstr.c
> @@ -865,7 +865,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
> name++, namesz--;
> else if ((no = (startswith(name, "no") != NULL))) {
> name += 2, namesz -= 2;
> - if (!*name) {
> + if (!*name || *name == ',') {
> match = 0;
> break; /* alone "no" keyword is error */
> }
>
> With this patch applied it also returns False for the "no,," pattern.
Applied, thanks!
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-28 11:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-25 11:43 [PATCH] libmount: Fix access to uninitialised value in mnt_optstr_locate_option soeren
2023-02-25 12:41 ` [PATCH v2] " soeren
2023-02-25 13:40 ` Sören Tempel
2023-02-27 10:50 ` Karel Zak
2023-02-27 19:00 ` Sören Tempel
2023-02-28 11:02 ` Karel Zak
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.