[PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[soeren@xxxxxxxxxxxxxxxxx]

From: Sören Tempel <soeren@xxxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxx>
---
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);