[PATCH v2] vfs: Improve overflow checking for stat*() compat fields

From: Sergey Klyaus
Date: Wed Oct 18 2017 - 12:05:24 EST

Next message: Steven Rostedt: "Re: [PATCH v2] static_key: Improve uninizialized key warning"
Previous message: Laurent Pinchart: "Re: [PATCH v2 02/15] usb: gadget: make config_item_type structures const"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

The checks for overflown fields in compat_statfs[64] structures are
heuristic and are not error prone. This patch introduces
ASSIGN_CHECK_OVERFLOW() family of macros which assign a field from a
kernel representation and check if value is get truncated or changed
its sign if the types of compat and in-kernel fields are different (and
if so, they set a flag to a "true").

These macros may use compiler builtin for overflow detection. They are
also used for stat*() syscalls.

Signed-off-by: Sergey Klyaus <sergey.m.klyaus@xxxxxxxxx>
---
This is replacement for vfs: fix statfs64() returning impossible EOVERFLOW for
64-bit f_files

fs/stat.c | 33 ++++----
fs/statfs.c | 195 +++++++++++++++++++++++++------------------
include/linux/build_bug.h | 10 +++
include/linux/compiler-gcc.h | 5 ++
include/linux/compiler.h | 43 ++++++++++
5 files changed, 190 insertions(+), 96 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index 8a6aa8c..e18539c 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -16,6 +16,7 @@
#include <linux/syscalls.h>
#include <linux/pagemap.h>
#include <linux/compat.h>
+#include <linux/compiler.h>

#include <linux/uaccess.h>
#include <asm/unistd.h>
@@ -207,6 +208,7 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta
{
static int warncount = 5;
struct __old_kernel_stat tmp;
+ bool offlag = false;

if (warncount > 0) {
warncount--;
@@ -219,12 +221,10 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta

memset(&tmp, 0, sizeof(struct __old_kernel_stat));
tmp.st_dev = old_encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
- tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
@@ -237,6 +237,7 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta
tmp.st_atime = stat->atime.tv_sec;
tmp.st_mtime = stat->mtime.tv_sec;
tmp.st_ctime = stat->ctime.tv_sec;
+
return copy_to_user(statbuf,&tmp,sizeof(tmp)) ? -EFAULT : 0;
}

@@ -295,6 +296,7 @@ SYSCALL_DEFINE2(fstat, unsigned int, fd, struct __old_kernel_stat __user *, stat
static int cp_new_stat(struct kstat *stat, struct stat __user *statbuf)
{
struct stat tmp;
+ bool offlag = false;

if (!valid_dev(stat->dev) || !valid_dev(stat->rdev))
return -EOVERFLOW;
@@ -305,12 +307,11 @@ static int cp_new_stat(struct kstat *stat, struct stat __user *statbuf)

INIT_STRUCT_STAT_PADDING(tmp);
tmp.st_dev = encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
@@ -431,6 +432,7 @@ SYSCALL_DEFINE3(readlink, const char __user *, path, char __user *, buf,
static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
{
struct stat64 tmp;
+ bool offlag = false;

INIT_STRUCT_STAT64_PADDING(tmp);
#ifdef CONFIG_MIPS
@@ -441,8 +443,8 @@ static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
tmp.st_dev = huge_encode_dev(stat->dev);
tmp.st_rdev = huge_encode_dev(stat->rdev);
#endif
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
#ifdef STAT64_HAS_BROKEN_ST_INO
tmp.__st_ino = stat->ino;
@@ -580,18 +582,17 @@ SYSCALL_DEFINE5(statx,
static int cp_compat_stat(struct kstat *stat, struct compat_stat __user *ubuf)
{
struct compat_stat tmp;
+ bool offlag = false;

if (!old_valid_dev(stat->dev) || !old_valid_dev(stat->rdev))
return -EOVERFLOW;

memset(&tmp, 0, sizeof(tmp));
tmp.st_dev = old_encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
- tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
diff --git a/fs/statfs.c b/fs/statfs.c
index fab9b6a..47cf963 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -8,8 +8,34 @@
#include <linux/security.h>
#include <linux/uaccess.h>
#include <linux/compat.h>
+#include <linux/compiler.h>
+#include <linux/bitops.h>
#include "internal.h"

+/**
+ * ASSIGN_CHECK_TRUNCATED_BITS() assigns src value to dst and checks if
+ * neither set bit in src lost during assignment. It uses fls()/fls64()
+ * to avoid sign extension induced by 32-bit -> 64-bit conversion.
+ */
+#define fls3264(value) \
+ ({ \
+ typeof(value) __val = (value); \
+ int __last_bit = 0; \
+ if (sizeof(__val) == 8) { \
+ __last_bit = fls64(__val); \
+ } else { \
+ __last_bit = fls(__val); \
+ } \
+ __last_bit; \
+ })
+#define ASSIGN_CHECK_TRUNCATED_BITS(dest, src, offlag) \
+ do { \
+ typeof(src) __src = (src); \
+ typeof(dest) __dst = (dest = __src); \
+ if (fls3264(__dst) != fls3264(__src)) \
+ offlag = true; \
+ } while (0)
+
static int flags_by_mnt(int mnt_flags)
{
int flags = 0;
@@ -110,39 +136,41 @@ static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
{
struct statfs buf;

- if (sizeof(buf) == sizeof(*st))
+ if (sizeof(buf) == sizeof(*st)) {
memcpy(&buf, st, sizeof(*st));
- else {
- if (sizeof buf.f_blocks == 4) {
- if ((st->f_blocks | st->f_bfree | st->f_bavail |
- st->f_bsize | st->f_frsize) &
- 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /*
- * f_files and f_ffree may be -1; it's okay to stuff
- * that into 32 bits
- */
- if (st->f_files != -1 &&
- (st->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (st->f_ffree != -1 &&
- (st->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
-
- buf.f_type = st->f_type;
- buf.f_bsize = st->f_bsize;
- buf.f_blocks = st->f_blocks;
- buf.f_bfree = st->f_bfree;
- buf.f_bavail = st->f_bavail;
- buf.f_files = st->f_files;
- buf.f_ffree = st->f_ffree;
- buf.f_fsid = st->f_fsid;
- buf.f_namelen = st->f_namelen;
- buf.f_frsize = st->f_frsize;
- buf.f_flags = st->f_flags;
+ } else {
+ bool offlag = false;
+ /* f_type can be signed 32-bits on some architectures, but it
+ * contains magic value, so we do not care if value overflows
+ * destination structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, st->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, st->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, st->f_bsize, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_blocks, st->f_blocks, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bfree, st->f_bfree, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bavail, st->f_bavail, offlag);
+ /* f_files and f_ffree may be -1; it's okay to put that into
+ * 32 bits
+ */
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_files, st->f_files, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_ffree, st->f_ffree, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[0], st->f_fsid.val[0]);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[1], st->f_fsid.val[1]);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, st->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, st->f_flags, offlag);
memset(buf.f_spare, 0, sizeof(buf.f_spare));
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
}
+
if (copy_to_user(p, &buf, sizeof(buf)))
return -EFAULT;
return 0;
@@ -247,32 +275,37 @@ SYSCALL_DEFINE2(ustat, unsigned, dev, struct ustat __user *, ubuf)
static int put_compat_statfs(struct compat_statfs __user *ubuf, struct kstatfs *kbuf)
{
struct compat_statfs buf;
- if (sizeof ubuf->f_blocks == 4) {
- if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
- kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /* f_files and f_ffree may be -1; it's okay
- * to stuff that into 32 bits */
- if (kbuf->f_files != 0xffffffffffffffffULL
- && (kbuf->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (kbuf->f_ffree != 0xffffffffffffffffULL
- && (kbuf->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
+ bool offlag = false;
+
memset(&buf, 0, sizeof(struct compat_statfs));
- buf.f_type = kbuf->f_type;
- buf.f_bsize = kbuf->f_bsize;
- buf.f_blocks = kbuf->f_blocks;
- buf.f_bfree = kbuf->f_bfree;
- buf.f_bavail = kbuf->f_bavail;
- buf.f_files = kbuf->f_files;
- buf.f_ffree = kbuf->f_ffree;
- buf.f_namelen = kbuf->f_namelen;
- buf.f_fsid.val[0] = kbuf->f_fsid.val[0];
- buf.f_fsid.val[1] = kbuf->f_fsid.val[1];
- buf.f_frsize = kbuf->f_frsize;
- buf.f_flags = kbuf->f_flags;
+
+ /* f_type can be signed 32-bits on some architectures, but it contains
+ * magic value, so we do not care if value overflows destination
+ * structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, kbuf->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, kbuf->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, kbuf->f_bsize, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_blocks, kbuf->f_blocks, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bfree, kbuf->f_bfree, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bavail, kbuf->f_bavail, offlag);
+ /* f_files and f_ffree may be -1; it's okay to put that into 32 bits */
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_files, kbuf->f_files, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_ffree, kbuf->f_ffree, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[0], kbuf->f_fsid.val[0]);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[1], kbuf->f_fsid.val[1]);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, kbuf->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, kbuf->f_flags, offlag);
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
if (copy_to_user(ubuf, &buf, sizeof(struct compat_statfs)))
return -EFAULT;
return 0;
@@ -303,32 +336,34 @@ COMPAT_SYSCALL_DEFINE2(fstatfs, unsigned int, fd, struct compat_statfs __user *,
static int put_compat_statfs64(struct compat_statfs64 __user *ubuf, struct kstatfs *kbuf)
{
struct compat_statfs64 buf;
- if (sizeof(ubuf->f_bsize) == 4) {
- if ((kbuf->f_type | kbuf->f_bsize | kbuf->f_namelen |
- kbuf->f_frsize | kbuf->f_flags) & 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /* f_files and f_ffree may be -1; it's okay
- * to stuff that into 32 bits */
- if (kbuf->f_files != 0xffffffffffffffffULL
- && (kbuf->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (kbuf->f_ffree != 0xffffffffffffffffULL
- && (kbuf->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
+ bool offlag = false;
+
memset(&buf, 0, sizeof(struct compat_statfs64));
- buf.f_type = kbuf->f_type;
- buf.f_bsize = kbuf->f_bsize;
- buf.f_blocks = kbuf->f_blocks;
- buf.f_bfree = kbuf->f_bfree;
- buf.f_bavail = kbuf->f_bavail;
- buf.f_files = kbuf->f_files;
- buf.f_ffree = kbuf->f_ffree;
- buf.f_namelen = kbuf->f_namelen;
- buf.f_fsid.val[0] = kbuf->f_fsid.val[0];
- buf.f_fsid.val[1] = kbuf->f_fsid.val[1];
- buf.f_frsize = kbuf->f_frsize;
- buf.f_flags = kbuf->f_flags;
+
+ /* f_type can be signed 32-bits on some architectures, but it contains
+ * magic value, so we do not care if value overflows destination
+ * structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, kbuf->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, kbuf->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, kbuf->f_bsize, offlag);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_blocks, kbuf->f_blocks);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_bfree, kbuf->f_bfree);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_bavail, kbuf->f_bavail);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_files, kbuf->f_files);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_ffree, kbuf->f_ffree);
+ ASSIGN_CHECK_OVERFLOW(buf.f_fsid.val[0], kbuf->f_fsid.val[0], offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_fsid.val[1], kbuf->f_fsid.val[1], offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, kbuf->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, kbuf->f_flags, offlag);
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
if (copy_to_user(ubuf, &buf, sizeof(struct compat_statfs64)))
return -EFAULT;
return 0;
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index b7d22d6..1d2f563 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -79,6 +79,16 @@
*/
#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")

+/**
+ * ASSIGN_CHECK_SAME_TYPE() -- assigns src value to dest but only if they have
+ * same type (will break build otherwise).
+ */
+#define ASSIGN_CHECK_SAME_TYPE(dest, src) \
+ do { \
+ BUILD_BUG_ON(!__same_type((dest), (src))); \
+ (dest) = (src); \
+ } while (0)
+
#endif /* __CHECKER__ */

#endif /* _LINUX_BUILD_BUG_H */
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 16d41de..71b4c29 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -305,6 +305,11 @@
#define __no_sanitize_address __attribute__((no_sanitize_address))
#endif

+#if GCC_VERSION >= 50000
+#define __ASSIGN_CHECK_OVERFLOW(dest, src) \
+ __builtin_add_overflow(0, (src), &dest)
+#endif
+
#if GCC_VERSION >= 50100
/*
* Mark structures as requiring designated initializers.
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f260ff3..6822b17 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -605,4 +605,47 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
(volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

+/**
+ * This is a helper macros for assignment of 32-bit fields in some syscall
+ * structures where kernel works with larger, 64-bit values and could induce
+ * overflow in 32-bit caller. If flag is set, it is expected that syscall
+ * will return -EOVERFLOW.
+ *
+ * Sizes of the fields are not compared here as it allows to validate for
+ * signedness difference in field types. Compiler will likely to eliminate
+ * overflow check if types of the fields are exactly matching.
+ *
+ * ASSIGN_CHECK_OVERFLOW_EXCEPT() is similar but allows to truncate a special
+ * magic constant (such as -1 represented as unsigned).
+ *
+ * @dest: name of the destination field
+ * @src: name of the source field
+ * @offlag: name of bool variable used to store overflow flag
+ * @value: magic value which is OK to be truncated
+ */
+#ifndef __ASSIGN_CHECK_OVERFLOW
+#define __ASSIGN_CHECK_OVERFLOW(dest, src) \
+ ({ \
+ typeof(src) ____src = (src); \
+ bool __offlag = false; \
+ (dest) = ____src; \
+ do { \
+ typeof(src) __val = dest; \
+ __offlag = (__val != ____src); \
+ } while (0); \
+ __offlag; \
+ })
+#endif
+#define ASSIGN_CHECK_OVERFLOW(dest, src, offlag) \
+ do { \
+ if (__ASSIGN_CHECK_OVERFLOW(dest, src)) \
+ offlag = true; \
+ } while (0)
+#define ASSIGN_CHECK_OVERFLOW_EXCEPT(dest, src, offlag, value) \
+ do { \
+ typeof(src) __src = (src); \
+ if (__ASSIGN_CHECK_OVERFLOW(dest, __src) && __src != (value)) \
+ offlag = true; \
+ } while (0)
+
#endif /* __LINUX_COMPILER_H */
--
2.10.2

Next message: Steven Rostedt: "Re: [PATCH v2] static_key: Improve uninizialized key warning"
Previous message: Laurent Pinchart: "Re: [PATCH v2 02/15] usb: gadget: make config_item_type structures const"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]