OS-7518: array over-read in has_saved_fp()


Issue Type:Bug
Priority:4 - Normal
Created at:2019-01-16T19:38:24.896Z
Updated at:2019-08-06T20:45:31.390Z


Created by:Former user
Reported by:Former user
Assigned to:Former user


Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2019-08-06T20:45:31.380Z)


The analysis for OS-7517 revealed -- albeit in a very strange way -- that has_saved_fp reads beyond the end of save_fp_pushes and save_fp_movs; here is the code for has_saved_fp:

static boolean_t
has_saved_fp(dis_handle_t *dhp, uint8_t *ins, int size)
        int             i, j;
        uint32_t        n;
        boolean_t       found_push = B_FALSE;
        ssize_t         sz = 0;

        for (i = 0; i < size; i += sz) {
                if ((sz = instr_size(dhp, ins, i, size)) < 1)
                        return (B_FALSE);

                if (found_push == B_FALSE) {
                        if (sz != 1) continue; 
                        n = INSTR1(ins, i);
                        for (j = 0; j <= NUM_FP_PUSHES; j++)
                                if (save_fp_pushes[j] == n) {
                                        found_push = B_TRUE;
                } else {
                        if (sz != 3)
                        n = INSTR3(ins, i);
                        for (j = 0; j <= NUM_FP_MOVS; j++)
                                if (save_fp_movs[j] == n)
                                        return (B_TRUE);

        return (B_FALSE);

And here are the definitions of save_fp_pushes and save_fp_movs:

static const uint8_t save_fp_pushes[] = {
        0x55,   /* pushq %rbp */
        0xcc    /* int $0x3 */
#define NUM_FP_PUSHES (sizeof (save_fp_pushes) / sizeof (save_fp_pushes[0]))

static const uint32_t save_fp_movs[] = {
        0x00e58948,     /* movq %rsp,%rbp, encoding 1 */
        0x00ec8b48,     /* movq %rsp,%rbp, encoding 2 */
#define NUM_FP_MOVS (sizeof (save_fp_movs) / sizeof (save_fp_movs[0]))

There is a clear off-by-one error in both loops; to dereference at index NUM_FP_PUSHES and NUM_FP_MOVS is to read off the end of the respective arrays.


Comment by Former user
Created at 2019-01-17T10:11:48.943Z

smatch knew about these:

/export/home/gk/src/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: saveargs.c:235 has_saved_fp() error: buffer overflow 'save_fp_pushes' 2 <= 2
/export/home/gk/src/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: saveargs.c:235 has_saved_fp() error: buffer overflow 'save_fp_pushes' 2 <= 2

Unfortunately this test is prone to false positives, so I just turned it off while going through and smatch-cleaning the entire source.

(No version of GCC is capable of warning about this.)

Comment by Jira Bot
Created at 2019-02-05T19:28:06.390Z

illumos-joyent commit 5b248548c9ca0108aa36e7e3db7fbc46077bcf4b (branch master, by Bryan Cantrill)

OS-7498 __ctype_mask[EOF] has been working by accident
OS-7511 undefined behavior in lx_cap_update_priv() turns gcc7 demonic
OS-7517 -faggressive-loop-optimizations is too aggressive
OS-7518 array over-read in has_saved_fp()
OS-7535 allow platform builds on base-64 image
OS-7536 stop shipping poold(1M) and its cohorts
Reviewed by: Robert Mustacchi <rm@joyent.com>
Reviewed by: John Levon <john.levon@joyent.com>
Approved by: Robert Mustacchi <rm@joyent.com>