Skip to content

Commit 23da752

Browse files
committed
gfx/drivers: fix OOM NULL-derefs and munmap(MAP_FAILED) in legacy platform drivers
Three related bugs in older embedded gfx backends: === gfx/drivers/dispmanx_gfx.c: dispmanx_surface_setup === *sp = calloc(1, sizeof(struct dispmanx_surface)); surface = *sp; surface->numpages = numpages; /* NULL-deref on OOM */ ... surface->pages = calloc(surface->numpages, sizeof(struct dispmanx_page)); for (i = 0; i < surface->numpages; i++) { surface->pages[i].used = false; /* NULL-deref on OOM */ ... } Two unchecked callocs back-to-back, each followed by unconditional field writes / array indexing. Function is void-returning, so callers can't see an error code - they just hand us an output pointer via 'sp' and trust it gets filled in. None of the three current callers (lines 367, 464, 518 - main/menu/back surface setup) check *sp after the call. === gfx/drivers/drm_gfx.c: drm_surface_setup === Identical sibling drift to the dispmanx bug above. The two routines share the same shape (output-ptr via sp, void return, outer surface calloc then inner pages calloc) and both were missing the same NULL checks. Fix (both files): NULL-check each calloc. On outer failure just return; on inner failure free the outer allocation and set *sp = NULL so callers get a consistent 'setup didn't produce a surface' signal. Note: this doesn't fully plug the OOM path - downstream helpers (dispmanx_surface_update_async, drm_surface_update, dispmanx_surface_free, drm_surface_free) do NOT NULL-check 'surface' either, so a caller that ignores *sp and then calls into those helpers would still crash. Fixing every call-site pattern is a bigger churn across the driver than this patch is trying to do; the surface_setup fix here at least stops setup itself from being the crash site and gives callers consistent post-condition semantics (*sp is NULL iff setup failed). === gfx/drivers/omap_gfx.c: omapfb_backup_state - munmap(MAP_FAILED) === pdata->saved_state->mem = malloc(pdata->saved_state->mi.size); mem = mmap(NULL, pdata->saved_state->mi.size, ...); if (!pdata->saved_state->mem || mem == MAP_FAILED) { RARCH_ERR("[Omap] Backup layer (mem backup) failed.\n"); munmap(mem, pdata->saved_state->mi.size); /* UB on MAP_FAILED */ return -1; } If mmap fails, 'mem' equals MAP_FAILED ((void*)-1). Calling munmap() on that is explicitly undefined behaviour per POSIX - glibc happens to return -1 with EINVAL today but there is no guarantee, and a future libc / sanitiser is entirely within its rights to trap. Fix: guard the munmap with 'if (mem != MAP_FAILED)'. The malloc-failure path (saved_state->mem == NULL) is separately fine: the memory never got allocated so there's nothing to munmap, and saved_state itself is freed later by omapfb_free() when the caller's fail_omapfb label runs. === Thread-safety === None of the three functions cross threads. dispmanx and drm surface setup run during video-driver init; omap backup_state also runs at init. No lock discipline changes. === Reachability === All three files are embedded-platform gfx backends with narrow audience: - dispmanx_gfx: Raspberry Pi legacy VideoCore IV - drm_gfx: bare-metal DRM/KMS (PiKISS, some retro handhelds) - omap_gfx: TI OMAP (N900, Pandora, older Beagleboard) OOM on these platforms is realistic - they typically have 128-512MB of total RAM and run on top of memory-tight minimal images. The dispmanx/drm OOM paths are hit on initial video init and also on every core-triggered surface re-setup for resolution changes.
1 parent b2e3dfd commit 23da752

3 files changed

Lines changed: 53 additions & 1 deletion

File tree

gfx/drivers/dispmanx_gfx.c

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,18 @@ static void dispmanx_surface_setup(struct dispmanx_video *_dispvars,
239239

240240
surface = *sp;
241241

242+
/* NULL-check the outer calloc: the next dozen lines of
243+
* surface->... field writes would NULL-deref on OOM. This
244+
* function is void-returning, so callers can't see an error
245+
* code - they have to notice *sp == NULL themselves. None of
246+
* the three current callers (lines 367, 464, 518) do that,
247+
* but letting the function no-op on OOM is still strictly
248+
* better than a segfault, and a subsequent
249+
* dispmanx_surface_update_async(..., *sp) would also no-op
250+
* on a NULL surface. */
251+
if (!surface)
252+
return;
253+
242254
/* Setup surface parameters */
243255
surface->numpages = numpages;
244256
/* We receive the pitch for what we consider "useful info",
@@ -255,6 +267,18 @@ static void dispmanx_surface_setup(struct dispmanx_video *_dispvars,
255267
* and initialize variables inside each page's struct. */
256268
surface->pages = calloc(surface->numpages, sizeof(struct dispmanx_page));
257269

270+
/* NULL-check the pages calloc as well. The for-loop below
271+
* indexes into surface->pages unconditionally. On OOM undo
272+
* the outer surface allocation so the caller gets a NULL
273+
* pointer back consistent with the initial-alloc failure
274+
* above. */
275+
if (!surface->pages)
276+
{
277+
free(surface);
278+
*sp = NULL;
279+
return;
280+
}
281+
258282
for (i = 0; i < surface->numpages; i++)
259283
{
260284
surface->pages[i].used = false;

gfx/drivers/drm_gfx.c

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,16 @@ static void drm_surface_setup(void *data, int src_width, int src_height,
232232

233233
surface = *sp;
234234

235+
/* NULL-check the outer calloc: the surface->... field writes
236+
* below would NULL-deref on OOM. Sibling bug to the one in
237+
* dispmanx_surface_setup - the two routines share this
238+
* structure (output-pointer + void return) and both had the
239+
* same missing checks. void-returning so callers can't see
240+
* an error code; letting the function no-op on OOM beats a
241+
* segfault. */
242+
if (!surface)
243+
return;
244+
235245
/* Setup surface parameters */
236246
surface->numpages = numpages;
237247
/* We receive the total pitch, including things that are
@@ -252,6 +262,16 @@ static void drm_surface_setup(void *data, int src_width, int src_height,
252262
surface->pages = (struct drm_page*)
253263
calloc(surface->numpages, sizeof(struct drm_page));
254264

265+
/* Same NULL-check for the pages array. Undo the outer
266+
* surface allocation on OOM to give callers a consistent
267+
* NULL. */
268+
if (!surface->pages)
269+
{
270+
free(surface);
271+
*sp = NULL;
272+
return;
273+
}
274+
255275
for (i = 0; i < surface->numpages; i++)
256276
{
257277
surface->pages[i].used = false;

gfx/drivers/omap_gfx.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,15 @@ static int omapfb_backup_state(omapfb_data_t *pdata)
370370
if (!pdata->saved_state->mem || mem == MAP_FAILED)
371371
{
372372
RARCH_ERR("[Omap] Backup layer (mem backup) failed.\n");
373-
munmap(mem, pdata->saved_state->mi.size);
373+
/* Only munmap if mmap actually succeeded. munmap(MAP_FAILED, ...)
374+
* is undefined per POSIX - MAP_FAILED is (void*)-1 and any
375+
* implementation-specific behaviour it triggers is no guarantee
376+
* against a future libc flagging it as an error or crashing.
377+
* The malloc failure path separately leaves saved_state->mem
378+
* as NULL; it gets cleaned up by omapfb_free() via the caller's
379+
* fail_omapfb goto. */
380+
if (mem != MAP_FAILED)
381+
munmap(mem, pdata->saved_state->mi.size);
374382
return -1;
375383
}
376384
memcpy(pdata->saved_state->mem, mem, pdata->saved_state->mi.size);

0 commit comments

Comments
 (0)