Actions
Bug #10470
closedarray over-read in has_saved_fp()
Start date:
2019-02-27
Due date:
% Done:
100%
Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:
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
Updated by John Levon over 3 years ago
See https://smartos.org/bugview/OS-7518 for more details
Updated by John Levon over 3 years ago
- Related to Feature #9996: use GCC 7 as default primary compiler added
Updated by Electric Monk over 3 years 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>
Actions