[LU-2074] Fix 'copy into fixed size buffer' errors

• Type: Technical task
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.5.0
• Affects Version/s: Lustre 2.4.0, Lustre 2.1.3
• Labels: build coverity ptr
• Assignee: Keith Mannthey (Inactive)
• Reporter: Sebastien Buisson (Inactive)

Description

Thanks to the Coverity tool, we found some 'security best practices violations' in the Lustre code, and more specifically cases of copy into fixed size buffer.
This is typically where we should use strlcpy() instead of strcpy() or strlcat() instead of strcat() because the size of the source buffer is not known.

I will propose a patch to address the issues.

Comments

[Peter Jones]

Landed for 2.5.0

[Dmitry Eremin (Inactive)]

Patch Set 16: Verified
Build Successful
http://build.whamcloud.com/job/lustre-reviews/16913/ : SUCCESS

[Keith Mannthey (Inactive)]

Dmitry, Can you confirm that patch works for lbuild and this 2074 patch?
I have been able to build local before just not in the lbuild/autotest framework.

[Dmitry Eremin (Inactive)]

The usage of strlcpy function brings additionally dependencies to userspace binaries. The following patch is required to build without error. I think it's a bad idea to use this function and add additional dependencies.

diff --git a/lnet/utils/Makefile.am b/lnet/utils/Makefile.am
index f11a21a..6c01d7a 100644
--- a/lnet/utils/Makefile.am
+++ b/lnet/utils/Makefile.am
@@ -70,17 +70,17 @@ endif
 wirecheck_SOURCES = wirecheck.c
 
 ptlctl_SOURCES = ptlctl.c
-ptlctl_LDADD =  -L. -lptlctl $(LIBCFSUTIL) $(LIBCFS) $(LIBREADLINE) $(LIBEFENCE)
+ptlctl_LDADD =  -L. -lptlctl $(LIBCFSUTIL) $(LIBCFS) $(LIBREADLINE) $(LIBEFENCE) $(PTHREAD_LIBS)
 ptlctl_DEPENDENCIES = libptlctl.a
 
 routerstat_SOURCES = routerstat.c
 
 debugctl_SOURCES = debugctl.c
-debugctl_LDADD = -L. -lptlctl $(LIBCFSUTIL) $(LIBREADLINE) $(LIBEFENCE)
+debugctl_LDADD = -L. -lptlctl $(LIBCFSUTIL) $(LIBCFS) $(LIBREADLINE) $(LIBEFENCE) $(PTHREAD_LIBS)
 debugctl_DEPENDENCIES = libptlctl.a
 
 lst_SOURCES = lst.c
-lst_LDADD = -L. -lptlctl $(LIBCFSUTIL) $(LIBCFS) $(LIBREADLINE) $(LIBEFENCE)
+lst_LDADD = -L. -lptlctl $(LIBCFSUTIL) $(LIBCFS) $(LIBREADLINE) $(LIBEFENCE) $(PTHREAD_LIBS)
 lst_DEPENDENCIES = libptlctl.a
 
 LND_LIBS =

diff --git a/lustre/utils/Makefile.am b/lustre/utils/Makefile.am
index 23bde35..a500d67 100644
--- a/lustre/utils/Makefile.am
+++ b/lustre/utils/Makefile.am
@@ -58,7 +58,7 @@ lustre_rsync_LDADD :=  liblustreapi.a $(LIBPTLCTL) $(PTHREAD_LIBS) $(LIBREADLINE
 lustre_rsync_DEPENDENCIES := $(LIBPTLCTL) liblustreapi.a
 
 ll_recover_lost_found_objs_SOURCES = ll_recover_lost_found_objs.c
-ll_recover_lost_found_objs_LDADD := $(LIBPTLCTL)
+ll_recover_lost_found_objs_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
 ll_recover_lost_found_objs_DEPENDENCIES := $(LIBPTLCTL)
 
 lshowmount_SOURCES = lshowmount.c nidlist.c nidlist.h
@@ -118,14 +118,14 @@ obdbarrier_SOURCES = obdbarrier.c obdiolib.c obdiolib.h
 req_layout_SOURCES = req-layout.c
 
 llog_reader_SOURCES = llog_reader.c
-llog_reader_LDADD := $(LIBPTLCTL)
+llog_reader_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
 llog_reader_DEPENDENCIES := $(LIBPTLCTL)
 
 lr_reader_SOURCES = lr_reader.c
 
 mount_lustre_SOURCES = mount_lustre.c mount_utils.c mount_utils.h
 mount_lustre_CPPFLAGS = $(AM_CPPFLAGS)
-mount_lustre_LDADD := $(LIBPTLCTL)
+mount_lustre_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
 mount_lustre_DEPENDENCIES := $(LIBPTLCTL)
 if LDISKFS_ENABLED
 mount_lustre_SOURCES += mount_utils_ldiskfs.c
@@ -139,7 +139,7 @@ endif
 
 mkfs_lustre_SOURCES = mkfs_lustre.c mount_utils.c mount_utils.h
 mkfs_lustre_CPPFLAGS = -UTUNEFS $(AM_CPPFLAGS)
-mkfs_lustre_LDADD := $(LIBPTLCTL)
+mkfs_lustre_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
 mkfs_lustre_DEPENDENCIES := $(LIBPTLCTL)
 if LDISKFS_ENABLED
 mkfs_lustre_SOURCES += mount_utils_ldiskfs.c
@@ -168,10 +168,12 @@ tunefs_lustre_LDFLAGS = -pthread -rdynamic -ldl
 endif
 
 l_getidentity_SOURCES = l_getidentity.c
-l_getidentity_LDADD := $(LIBPTLCTL)
+l_getidentity_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
 l_getidentity_DEPENDENCIES := $(LIBPTLCTL)
 
 ltrack_stats_SOURCES = ltrack_stats.c
+ltrack_stats_LDADD := $(LIBPTLCTL) $(PTHREAD_LIBS)
+ltrack_stats_DEPENDENCIES := $(LIBPTLCTL)
 
 EXTRA_DIST = $(sbin_scripts) $(bin_scripts)
 
diff --git a/lustre/utils/ltrack_stats.c b/lustre/utils/ltrack_stats.c
index eed0fcc..de5a3d6 100644
--- a/lustre/utils/ltrack_stats.c
+++ b/lustre/utils/ltrack_stats.c
@@ -45,7 +45,9 @@
 #include <signal.h>
 #include <string.h>
 #include <stdlib.h>
+#include <errno.h>
 
+extern size_t strlcpy(char *dst, const char *src, size_t len);
 
 #define TRACK_BY_GID 0
 #define TRACK_BY_PPID 1

[Keith Mannthey (Inactive)]

This issue is blocked on a build issue.
http://jira.whamcloud.com/browse/LU-2662

[Sebastien Buisson (Inactive)]

The initial purpose of the patch was to avoid some buffer overflow issues found by Coverity. But I agree we could try to detect cases of truncation, and return an E2BIG error instead of continuing with partial data.
Hopefully strlcpy() and strlcat() make truncation detection easy. Please tell me if you can find out what is going on with the compilation issue.

Cheers,
Sebastien.

[Keith Mannthey (Inactive)]

Sabastien as per our discussion in review.

I will look in the compilation issue. Lustre has both kernel and userspace context I will look around and see what the issue is.

Is it safe to say that:
In general the spots that Coverity found have unsafe data types where the receiving buffer may be smaller then the sending buffer? I agree char * can be vague. As mentioned some spots are safe and not identified by the tool and we are protecting against overflows where we will go and trample on neighboring data. What this patch seems to be attempting to do is truncate the message down to the receiving message size.

What happens when a message does get truncated? Likely a user of the data will have a random failure as the data is incomplete. Some data is lost. I would agree that truncation is better than overflow and random corruption. It seems there is at least a 3rd option which is audit and see if some of these tricky situations can be resolved through fundamental code changes and tricky spots add level of checking and add E2BIG errors when we will truncate. If we stop fitting into the receiving buffers I would rather we raise an error and stop than continue on with only part of the data.

Does this seem reasonable?

[Keith Mannthey (Inactive)]

I will take a look at it.

[Sebastien Buisson (Inactive)]

The patch is available here:

http://review.whamcloud.com/4154

Could you please review it?