Project

General

Profile

Bug #10470

array over-read in has_saved_fp()

Added by John Levon 8 months ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
2019-02-27
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage

Description

The analysis for OS-7517 (#10469) 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;
                                        break;
                                }
                } else {
                        if (sz != 3)
                                continue;
                        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.

This bug mirrors Joyent bug OS-7518


Related issues

Related to illumos gate - Feature #9996: use GCC 7 as default primary compilerClosed2018-11-21

Actions

History

#1

Updated by John Levon 8 months ago

#2

Updated by John Levon 8 months ago

  • Related to Feature #9996: use GCC 7 as default primary compiler added
#3

Updated by Joshua M. Clulow 8 months ago

  • Description updated (diff)
#4

Updated by Electric Monk 8 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 92c1a61163ff6a0655b27bd429856e171e7ce5f5

commit  92c1a61163ff6a0655b27bd429856e171e7ce5f5
Author: Bryan Cantrill <bryan@joyent.com>
Date:   2019-03-01T11:11:54.000Z

    10468 __ctype_mask[EOF] has been working by accident
    10469 GCC's -faggressive-loop-optimizations is too aggressive
    10470 array over-read in has_saved_fp()
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Reviewed by: John Levon <john.levon@joyent.com>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Igor Kozhukhov <igor@dilos.org>
    Reviewed by: Andy Fiddaman <andy@omniosce.org>
    Reviewed by: Gergő Doma <domag02@gmail.com>
    Reviewed by: Gary Mills <gary_mills@fastmail.fm>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF