[PATCH 0/5] Hardenize libibverbs initialisation

[PATCH 0/5] Hardenize libibverbs initialisation

[Yann Droneaud]

In order to hardenize my test application, I tried "fuzzing" its input
with zzuf[1]. But my application mostly failed to initialize due to
unhandled failure cases.

So here're are patches to clean the situation and let's application
exit properly.

The patches here are against kernel.org's libibverbs repository[2]

[1] http://caca.zoy.org/wiki/zzuf
[2] http://git.kernel.org/?p=libs/infiniband/libibverbs.git

Yann Droneaud (5):
  load_drivers: reset the list head after releasing items
  ibverbs_init: reset the list head after releasing items
  read_config: don't try to open file beginning with '.'
  read_config: ignore directory entry with backup suffix (~)
  read_config_file: ignore driver line without driver name

 src/init.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] load_drivers: reset the list head after releasing items

[Yann Droneaud]

In some failure cases, load_drivers() could be called multiple times.
Each time, it will try to free item from driver_name_list, but it doesn't
reset the list head.

Spotted using zzuf.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/init.c b/src/init.c
index 4f0130e..9e235e8 100644
--- a/src/init.c
+++ b/src/init.c
@@ -232,6 +232,8 @@ static void load_drivers(void)
 		free(name->name);
 		free(name);
 	}
+
+	driver_name_list = NULL;
 }
 
 static void read_config_file(const char *path)
-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ibverbs_init: reset the list head after releasing items

[Yann Droneaud]

In some cases, ibverbs_init() could be called multiple times.
After the first time, find_sysfs_devs() will add sysfs dev entry
to the invalid sysfs_dev_list head.
If you're really unlucky, the last sysfs_dev->next will point to sysfs_dev,
and ibverbs_init() will be stuck in an infinite loop.

Spotted using zzuf.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/init.c b/src/init.c
index 9e235e8..bdc1634 100644
--- a/src/init.c
+++ b/src/init.c
@@ -539,5 +539,7 @@ out:
 		free(sysfs_dev);
 	}
 
+	sysfs_dev_list = NULL;
+
 	return num_devices;
 }
-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] read_config: don't try to open file beginning with '.'

[Yann Droneaud]

Files beginning with a dot are mostly current and parent directories or,
by convention, hidden files.

Those path are skipped in find_sysfs_dev().

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/init.c b/src/init.c
index bdc1634..46ff24c 100644
--- a/src/init.c
+++ b/src/init.c
@@ -308,6 +308,9 @@ static void read_config(void)
 	while ((dent = readdir(conf_dir))) {
 		struct stat buf;
 
+		if (dent->d_name[0] == '.')
+			continue;
+
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {
 			fprintf(stderr, PFX "Warning: couldn't read config file %s/%s.\n",
 				IBV_CONFIG_DIR, dent->d_name);
-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] read_config: don't try to open file beginning with '.'

[Roland Dreier]

On Thu, Mar 31, 2011 at 2:29 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Files beginning with a dot are mostly current and parent directories or,
> by convention, hidden files.
>
> Those path are skipped in find_sysfs_dev().

Not sure we want to do this. We already skip non-regular files, and
I'm not sure we want to exclude certain file names.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] read_config: don't try to open file beginning with '.'

[Yann Droneaud]

Hi,

Le mardi 31 mai 2011 à 11:42 -0700, Roland Dreier a écrit :
> On Thu, Mar 31, 2011 at 2:29 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> > Files beginning with a dot are mostly current and parent directories or,
> > by convention, hidden files.
> >
> > Those path are skipped in find_sysfs_dev().
> 
> Not sure we want to do this. We already skip non-regular files, and
> I'm not sure we want to exclude certain file names.

For example, while testing libibverbs config parsing, I've used emacs to
edit drivers file. And, when emacs open a buffer, it create a lock file
with a leading dot.

So running a libibverbs program prints
libibverbs: Warning: couldn't stat config file
'/tmp/ib/etc/libibverbs.d/.#test'.

Here's a strace output:

open("/tmp/ib/etc/libibverbs.d", O_RDONLY|O_NONBLOCK|O_DIRECTORY|
O_CLOEXEC) = 3
getdents(3, /* 3 entries */, 32768)     = 80
stat("/tmp/ib/etc/libibverbs.d/.#test", 0x7fffe11e8c60) = -1 ENOENT (No
such file or directory)
write(2, "libibverbs: Warning: couldn't st"..., 82) = 82
stat("/tmp/ib/etc/libibverbs.d/.", {st_mode=S_IFDIR|0775,
st_size=4096, ...}) = 0
stat("/tmp/ib/etc/libibverbs.d/..", {st_mode=S_IFDIR|0775,
st_size=4096, ...}) = 0
getdents(3, /* 0 entries */, 32768)     = 0
close(3)                                = 0


Skipping that "normally" hidden file seems a good behavior. Same thing
for those backup files with ~ suffix.

It's mostly harmless, but code which load dynamic modules makes me a
little angry :)

For example, directory libibverbs.d should be owned by root with write
limited write permission, likewise for the driver files.

Regards.

-- 
Yann Droneaud


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] read_config: ignore directory entry with backup suffix (~)

[Yann Droneaud]

Try to protect libibverbs from hand modified configuration files.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/init.c b/src/init.c
index 46ff24c..dd6fb00 100644
--- a/src/init.c
+++ b/src/init.c
@@ -311,6 +311,9 @@ static void read_config(void)
 		if (dent->d_name[0] == '.')
 			continue;
 
+		if (dent->d_name[0] == '\0' || dent->d_name[strlen(dent->d_name) - 1] == '~')
+			continue;
+
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {
 			fprintf(stderr, PFX "Warning: couldn't read config file %s/%s.\n",
 				IBV_CONFIG_DIR, dent->d_name);
-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] read_config_file: ignore driver line without driver name

[Yann Droneaud]

If there's no driver name, strsep() will set config to NULL and later
process of driver name will segfault.

Spotted with zzuf.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/init.c b/src/init.c
index dd6fb00..4d3d081 100644
--- a/src/init.c
+++ b/src/init.c
@@ -259,7 +259,7 @@ static void read_config_file(const char *path)
 
 		field = strsep(&config, "\n\t ");
 
-		if (strcmp(field, "driver") == 0) {
+		if (strcmp(field, "driver") == 0 && config != NULL) {
 			struct ibv_driver_name *driver_name;
 
 			config += strspn(config, "\t ");
-- 
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] read_config_file: ignore driver line without driver name

[Roland Dreier]

On Thu, Mar 31, 2011 at 2:29 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> If there's no driver name, strsep() will set config to NULL and later
> process of driver name will segfault.

Thanks, applied this one.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html