fortify: Use WARN instead of BUG for now

tools/objtool/check.c static int __dead_end_function(struct objtool_file *file, struct symbol *func, · line 160

Quoted 9 lines · Kees Cook

tools/objtool/check.c +1 -1

@@ -156,7 +156,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
              "kvm_spurious_fault",
              "__reiserfs_panic",
              "lbug_with_loc",
-             "fortify_panic",
+             "fortify_overflow",
      };

If the function is no longer '__noreturn' then this entry needs to be removed from the global_noreturns list.

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning in adfs about a possible buffer overflow:

In function 'memcpy',

    inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
    inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
    __read_overflow2();
    ^~~~~~~~~~~~~~~~~~

The warning is correct in the sense that a negative 'pos' argument to the function would have that result. However, this is not a bug, as we know the position is always positive (in fact, between 5 and 2007, inclusive) when the function gets called.

Changing the variable to a unsigned type avoids the problem. I decided to use 'unsigned int' for the position in the directory and the block number, as they are both counting things, but use size_t for the offset and length that get passed into memcpy. This shuts up the warning.

Signed-off-by Arnd Bergmann <[email protected]>

Quoted 21 lines · Arnd Bergmann

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning in adfs about a possible buffer overflow:

In function 'memcpy',

    inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
    inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
    __read_overflow2();
    ^~~~~~~~~~~~~~~~~~

The warning is correct in the sense that a negative 'pos' argument to the function would have that result. However, this is not a bug, as we know the position is always positive (in fact, between 5 and 2007, inclusive) when the function gets called.

Changing the variable to a unsigned type avoids the problem. I decided to use 'unsigned int' for the position in the directory and the block number, as they are both counting things, but use size_t for the offset and length that get passed into memcpy. This shuts up the warning.

Quoted 1 line

Signed-off-by Arnd Bergmann <[email protected]>

Acked-by Kees Cook <[email protected]>

Thanks for the fix! (Added sfr to Cc since he noticed this too.)

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning about possible stack overflow from a memcpy that was not strictly bounded to the size of the local variable:

    inlined from 'ext4_mb_seq_groups_show' at fs/ext4/mballoc.c:2322:2:

include/linux/string.h:309:9: error: '__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]

We actually had a bug here that would have been found by the warning, but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix stack memory corruption with 64k block size").

This replaces the fixed-length structure on the stack with a variable-length structure, using the correct upper bound that tells the compiler that everything is really fine here. I also change the loop count to check for the same upper bound for consistency, but the existing code is already correct here.

Note that while clang won't allow certain kinds of variable-length arrays in structures, this particular instance is fine, as the array is at the end of the structure, and the size is strictly bounded.

There is one remaining issue with the function that I'm not addressing
here: With s_blocksize_bits==16, we don't actually print the last two members of the array, as we loop though just the first 14 members. This could be easily addressed by adding two extra columns in the output, but that could in theory break parsers in user space, and should be a separate patch if we decide to modify it.

Signed-off-by Arnd Bergmann <[email protected]>

---
 fs/ext4/mballoc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

fs/ext4/mballoc.c +6 -6

index 581e357e8406..803cab1939fe 100644
@@ -2295,9 +2295,12 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
        int err, buddy_loaded = 0;
        struct ext4_buddy e4b;
        struct ext4_group_info *grinfo;
+       unsigned char blocksize_bits = min_t(unsigned char,
+                                            sb->s_blocksize_bits,
+                                            EXT4_MAX_BLOCK_LOG_SIZE);
        struct sg {
                struct ext4_group_info info;
-               ext4_grpblk_t counters[EXT4_MAX_BLOCK_LOG_SIZE + 2];
+               ext4_grpblk_t counters[blocksize_bits + 2];
        } sg;
 
        group--;

@@ -2306,8 +2309,6 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
                              " 2^0   2^1   2^2   2^3   2^4   2^5   2^6  "
                              " 2^7   2^8   2^9   2^10  2^11  2^12  2^13  ]\n");
 
-       i = (sb->s_blocksize_bits + 2) * sizeof(sg.info.bb_counters[0]) +
-               sizeof(struct ext4_group_info);
        grinfo = ext4_get_group_info(sb, group);
        /* Load the group info in memory only if not already loaded. */
        if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) {

@@ -2319,7 +2320,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
                buddy_loaded = 1;
        }
 
-       memcpy(&sg, ext4_get_group_info(sb, group), i);
+       memcpy(&sg, ext4_get_group_info(sb, group), sizeof(sg));
 
        if (buddy_loaded)
                ext4_mb_unload_buddy(&e4b);

@@ -2327,7 +2328,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
        seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
                        sg.info.bb_fragments, sg.info.bb_first_free);
        for (i = 0; i <= 13; i++)
-               seq_printf(seq, " %-5u", i <= sb->s_blocksize_bits + 1 ?
+               seq_printf(seq, " %-5u", i <= blocksize_bits + 1 ?
                                sg.info.bb_counters[i] : 0);
        seq_printf(seq, " ]\n");
 
--

2.9.0

There is one remaining issue with the function that I'm not addressing
here: With s_blocksize_bits==16, we don't actually print the last two members of the array, as we loop though just the first 14 members. This could be easily addressed by adding two extra columns in the output, but that could in theory break parsers in user space, and should be a separate patch if we decide to modify it.

Quoted 6 lines · Arnd Bergmann

There is one remaining issue with the function that I'm not addressing
here: With s_blocksize_bits==16, we don't actually print the last two members of the array, as we loop though just the first 14 members. This could be easily addressed by adding two extra columns in the output, but that could in theory break parsers in user space, and should be a separate patch if we decide to modify it.

Actually, the counters array is blocksize_bits+2 in length. So for all block sizes greater than 4k (blocksize_bits == 12), we're not iterating over all of the free space counters maintained by mballoc.

However, since most Linux systems run architectures where the page size is 4k, and the Linux VM really doesn't easily support file system block sizes greater than the page size, this really isn't an issue except on Itanics and Power systems.

Thanks, I'll apply this patch.

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning about possible stack overflow from a memcpy that was not strictly bounded to the size of the local variable:

    inlined from 'ext4_mb_seq_groups_show' at fs/ext4/mballoc.c:2322:2:

include/linux/string.h:309:9: error: '__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]

We actually had a bug here that would have been found by the warning, but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix stack memory corruption with 64k block size").

This replaces the fixed-length structure on the stack with a variable-length structure, using the correct upper bound that tells the compiler that everything is really fine here. I also change the loop count to check for the same upper bound for consistency, but the existing code is already correct here.

Note that while clang won't allow certain kinds of variable-length arrays in structures, this particular instance is fine, as the array is at the end of the structure, and the size is strictly bounded.

There is one remaining issue with the function that I'm not addressing
here: With s_blocksize_bits==16, we don't actually print the last two members of the array, as we loop though just the first 14 members. This could be easily addressed by adding two extra columns in the output, but that could in theory break parsers in user space, and should be a separate patch if we decide to modify it.

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning about possible stack overflow from a memcpy that was not strictly bounded to the size of the local variable:

inlined from 'ext4_mb_seq_groups_show' at
fs/ext4/mballoc.c:2322:2: include/linux/string.h:309:9: error:
'__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]

We actually had a bug here that would have been found by the warning, but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix stack memory corruption with 64k block size").

This replaces the fixed-length structure on the stack with a variable-length structure, using the correct upper bound that tells the compiler that everything is really fine here. I also change the loop count to check for the same upper bound for consistency, but the existing code is already correct here.

Note that while clang won't allow certain kinds of variable-length arrays in structures, this particular instance is fine, as the array is at the end of the structure, and the size is strictly bounded.

Hi Arnd,

Quoted 4 lines

Note that while clang won't allow certain kinds of variable-length arrays in structures, this particular instance is fine, as the array is at the end of the structure, and the size is strictly bounded.

Unfortunately it doesn't appear to work, at least with ppc64le clang:

fs/ext4/mballoc.c:2303:17: error: fields must have a constant size: 'variable length array in structure' extension will never be supported

        ext4_grpblk_t counters[blocksize_bits + 2];

Quoted 7 lines

Unfortunately it doesn't appear to work, at least with ppc64le clang:

fs/ext4/mballoc.c:2303:17: error: fields must have a constant size:
'variable length array in structure' extension will never be supported ext4_grpblk_t counters[blocksize_bits + 2];

My fix for this is in the ext4/dev branch in linux-next, I hope it still makes it into v4.13.