From ffae9ae2459ad306de9435dd150fc7525e068a50 Mon Sep 17 00:00:00 2001 From: Mark Pizzolato Date: Wed, 21 Sep 2022 08:44:43 -0700 Subject: [PATCH] SCP: Add more robust register checking to avoid out of bounds array references --- scp.c | 34 +++++++++++++++++++++++----------- sim_defs.h | 18 +++++++++++++----- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/scp.c b/scp.c index 39403bc2..fe17a233 100644 --- a/scp.c +++ b/scp.c @@ -16303,9 +16303,9 @@ for (i = 0; (dptr = devices[i]) != NULL; i++) { } if (sim_switches & SWMASK ('R')) /* Debug output */ - sim_printf ("%5s:%-9.9s %s%s%s(rdx=%u, wd=%u, off=%u, dep=%u, strsz=%u, objsz=%u, elesz=%u, rsz=%u, %s%s%s membytes=%u)\n", + sim_printf ("%5s:%-9.9s %s%s%s(rdx=%u, wd=%u, off=%u, dep=%u, strsz=%u, objsz=%u, oobjsz=%u, elesz=%u, rsz=%u, %s%s%s membytes=%u)\n", dptr->name, rptr->name, rptr->desc ? rptr->desc : "", rptr->desc ? "\n\t" : "", rptr->macro ? rptr->macro : "", - rptr->radix, rptr->width, rptr->offset, rptr->depth, (uint32)rptr->stride, (uint32)rptr->obj_size, (uint32)rptr->size, rsz, + rptr->radix, rptr->width, rptr->offset, rptr->depth, (uint32)rptr->stride, (uint32)rptr->obj_size, (uint32)rptr->pobj_size, (uint32)rptr->size, rsz, (rptr->flags & REG_VMAD) ? " REG_VMAD" : "", (rptr->flags & REG_VMIO) ? " REG_VMIO" : "", (rptr->flags & REG_DEPOSIT) ? " REG_DEPOSIT" : "", memsize); @@ -16318,11 +16318,20 @@ for (i = 0; (dptr = devices[i]) != NULL; i++) { } else { if (rptr->depth > 1) { - Mprintf (f, "%s %s:%s used the %s macro to describe a %u bit%s wide and %u elements deep array\n", sim_name, dptr->name, rptr->name, rptr->macro, rptr->width, (rptr->width == 1) ? "" : "s", rptr->depth); + Mprintf (f, "%s %s:%s used the %s macro to describe a %u bit%s wide and %u element deep array\n", sim_name, dptr->name, rptr->name, rptr->macro, rptr->width, (rptr->width == 1) ? "" : "s", rptr->depth); if (rptr->stride == 0) { Bad = TRUE; Mprintf (f, "%s %s:%s array stride is unspecified\n", sim_name, dptr->name, rptr->name); } + if (((((size_t)rptr->loc + rptr->stride)) % rsz) != 0) { + Bad = TRUE; + Mprintf (f, "%s %s:%s array seems to not have naturally aligned elements with a stride of %u and depth of %u\n", sim_name, dptr->name, rptr->name, (uint32)rptr->stride, (uint32)rptr->depth); + } + if ((memcmp (rptr->macro, "BRDATA", 6) == 0) && + ((rptr->depth * rptr->stride) > rptr->obj_size)) { + Bad = TRUE; + Mprintf (f, "%s %s:%s stride of %u and depth of %u is unreasonable and access will reach beyond the array upper bound\n", sim_name, dptr->name, rptr->name, (uint32)rptr->stride, (uint32)rptr->depth); + } } else { /* rptr->depth == 0 */ Bad = TRUE; @@ -16370,20 +16379,20 @@ for (i = 0; (dptr = devices[i]) != NULL; i++) { } else { bytes *= rptr->depth; - if (!Bad) - if ((rsz * rptr->depth == rptr->obj_size) || - ((rptr->macro && (strstr (rptr->macro, "STRDATA") != NULL)) && (rsz <= rptr->obj_size)) || + if (!Bad) + if ((rsz * rptr->depth == rptr->pobj_size) || + ((rptr->macro && (strstr (rptr->macro, "STRDATA") != NULL)) && (rsz <= rptr->pobj_size)) || ((rptr->depth == 1) && - ((rptr->obj_size == sizeof (t_value)) || (rsz < rptr->obj_size))) || - ((rptr->depth != 1) && (bytes == rptr->obj_size)) || + ((rptr->pobj_size == sizeof (t_value)) || (rsz < rptr->pobj_size))) || + ((rptr->depth != 1) && (bytes == rptr->pobj_size)) || ((rptr->depth != 1) && (rptr->offset == 0) && (rptr->width == 8) && - ((rptr->depth == rptr->obj_size) || (rptr->depth == rptr->obj_size - 1))) || - ((rptr->depth != 1) && (rptr->offset == 0) && (rptr->obj_size == rptr->size))) + ((rptr->depth == rptr->obj_size) || (rptr->depth == rptr->pobj_size - 1))) || + ((rptr->depth != 1) && (rptr->offset == 0) && (rptr->pobj_size == rptr->size))) continue; Bad = TRUE; Mprintf (f, "\ttherefore EXAMINE and SAVE operations will reference %u byte%s\n", bytes, (bytes != 1) ? "s" : ""); Mprintf (f, "\tand DEPOSIT and RESTORE operations will affect %u byte%s of memory\n", bytes, (bytes != 1) ? "s" : ""); - Mprintf (f, "\twhile the variable lives in %u bytes of memory\n", (uint32)rptr->obj_size); + Mprintf (f, "\twhile the variable lives in %u bytes of memory\n", (uint32)rptr->pobj_size); } } else { @@ -16433,6 +16442,9 @@ static struct validation_test { REG reg[7]; t_stat expected_result; } validations[] = { + { { { BRDATAD (STRUCT, validate_units, 16, 8*sizeof(treg16), sizeof(validate_units)/sizeof(treg16), "an invalid array of scalars") }, + {NULL} }, + SCPE_IERR}, { { { SRDATAD (AMBIG, validate_units, pos, 10, 32, 0, 3, "a basic register array") }, { SRDATAD (AMBIG, validate_units, u3, 10, 32, 0, 3, "an ambiguously named register array") }, {NULL} }, diff --git a/sim_defs.h b/sim_defs.h index 83a2c96f..a3ca8635 100644 --- a/sim_defs.h +++ b/sim_defs.h @@ -702,7 +702,8 @@ struct REG { BITFIELD *fields; /* bit fields */ uint32 qptr; /* circ q ptr */ size_t stride; /* structure/object size (for indexing) */ - size_t obj_size; /* sizeof(*loc) */ + size_t obj_size; /* sizeof(loc) */ + size_t pobj_size; /* sizeof(*loc) */ size_t size; /* sizeof(**loc) or sizeof(*loc) if depth == 1 */ const char *macro; /* Initializer Macro Name */ const char *source_file; /* source file used macro */ @@ -944,8 +945,9 @@ struct MEMFILE { FLDATA Scalar with single bit display/entry GRDATA Scalar with with specification of radix/width/offset parameters - BRDATA Singly-subscripted array - CRDATA Doubly-subscripted array + BRDATA Singly-subscripted array of scalars + CRDATA Doubly-subscripted array of scalars + VBRDATA List of elements accessed like a Singly-subscripted array of scalars SRDATA Singly-subscripted array of general structure fields URDATA Singly-subscripted array of UNIT structure fields @@ -1016,12 +1018,14 @@ struct MEMFILE { 5. The SAVEDATA macro is useful to indicate global variables whose values must persist across a SAVE and RESTORE. Such data is hidden from the - register user interface. + register user interface. Data saved this way may generally not be + usefully restored when a SAVE and RESTORE operations are done on host + systems with different endian attributes. */ /* Internal use ONLY (see below) Generic Register declaration for all fields */ #define _RegCheck(nm,loc,rdx,wd,off,dep,desc,flds,qptr,siz,elesiz,macro) \ - nm, (loc), (rdx), (wd), (off), (dep), (desc), (flds), (qptr), (siz), sizeof(*(loc)), (elesiz), #macro, __FILE__, __LINE__ + nm, (loc), (rdx), (wd), (off), (dep), (desc), (flds), (qptr), (siz), sizeof(loc), sizeof(*(loc)), (elesiz), #macro, __FILE__, __LINE__ /* Generic Register declaration for all fields. If the register structure is extended, this macro will be retained and a @@ -1031,6 +1035,10 @@ struct MEMFILE { /* v3 compatible macro */ #define XRDATA(nm,loc,rdx,wd,off,dep,siz,str) \ _RegCheck(#nm,loc,rdx,wd,off,dep,NULL,NULL,0,siz,sizeof((loc)),XRDATA) +#define XRDATAD(nm,loc,rdx,wd,off,dep,siz,str,desc) \ + _RegCheck(#nm,loc,rdx,wd,off,dep,desc,NULL,0,siz,sizeof((loc)),XRDATAD) +#define XRDATADF(nm,loc,rdx,wd,off,dep,siz,str,desc,flds) \ + _RegCheck(#nm,loc,rdx,wd,off,dep,desc,flds,0,siz,sizeof((loc)),XRDATADF) /* Right Justified Octal Register Data */ #define ORDATA(nm,loc,wd) \