From d040478d8aad7a05aac98651ec6ac5c2e9265a22 Mon Sep 17 00:00:00 2001 From: Jim Mussared Date: Wed, 4 Oct 2023 22:13:18 +1100 Subject: [PATCH] extmod/modframebuf: Validate FrameBuffer bounds against input buffer. This ensures that the buffer is large enough for the specified width, height, bits-per-pixel, and stride. Also makes the legacy FrameBuffer1 constructor re-use the FrameBuffer make_new to save some code size. Fixes issue #12562. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared --- extmod/modframebuf.c | 74 +++++++++++++++-------------- tests/extmod/framebuf_bounds.py | 60 +++++++++++++++++++++++ tests/extmod/framebuf_bounds.py.exp | 44 +++++++++++++++++ 3 files changed, 143 insertions(+), 35 deletions(-) create mode 100644 tests/extmod/framebuf_bounds.py create mode 100644 tests/extmod/framebuf_bounds.py.exp diff --git a/extmod/modframebuf.c b/extmod/modframebuf.c index 87f7609dd8..eabe819973 100644 --- a/extmod/modframebuf.c +++ b/extmod/modframebuf.c @@ -273,42 +273,59 @@ STATIC void fill_rect(const mp_obj_framebuf_t *fb, int x, int y, int w, int h, u STATIC mp_obj_t framebuf_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args_in) { mp_arg_check_num(n_args, n_kw, 4, 5, false); - mp_obj_framebuf_t *o = mp_obj_malloc(mp_obj_framebuf_t, type); - o->buf_obj = args_in[0]; + mp_int_t width = mp_obj_get_int(args_in[1]); + mp_int_t height = mp_obj_get_int(args_in[2]); + mp_int_t format = mp_obj_get_int(args_in[3]); + mp_int_t stride = n_args >= 5 ? mp_obj_get_int(args_in[4]) : width; - mp_buffer_info_t bufinfo; - mp_get_buffer_raise(args_in[0], &bufinfo, MP_BUFFER_WRITE); - o->buf = bufinfo.buf; - - o->width = mp_obj_get_int(args_in[1]); - o->height = mp_obj_get_int(args_in[2]); - o->format = mp_obj_get_int(args_in[3]); - if (n_args >= 5) { - o->stride = mp_obj_get_int(args_in[4]); - } else { - o->stride = o->width; + if (width < 1 || height < 1 || width > 0xffff || height > 0xffff || stride > 0xffff || stride < width) { + mp_raise_ValueError(NULL); } - switch (o->format) { + size_t height_required = height; + size_t bpp = 1; + + switch (format) { case FRAMEBUF_MVLSB: - case FRAMEBUF_RGB565: + height_required = (height + 7) & ~7; break; case FRAMEBUF_MHLSB: case FRAMEBUF_MHMSB: - o->stride = (o->stride + 7) & ~7; + stride = (stride + 7) & ~7; break; case FRAMEBUF_GS2_HMSB: - o->stride = (o->stride + 3) & ~3; + stride = (stride + 3) & ~3; + bpp = 2; break; case FRAMEBUF_GS4_HMSB: - o->stride = (o->stride + 1) & ~1; + stride = (stride + 1) & ~1; + bpp = 4; break; case FRAMEBUF_GS8: + bpp = 8; + break; + case FRAMEBUF_RGB565: + bpp = 16; break; default: mp_raise_ValueError(MP_ERROR_TEXT("invalid format")); } + mp_buffer_info_t bufinfo; + mp_get_buffer_raise(args_in[0], &bufinfo, MP_BUFFER_WRITE); + + if (height_required * stride * bpp / 8 > bufinfo.len) { + mp_raise_ValueError(NULL); + } + + mp_obj_framebuf_t *o = mp_obj_malloc(mp_obj_framebuf_t, type); + o->buf_obj = args_in[0]; + o->buf = bufinfo.buf; + o->width = width; + o->height = height; + o->format = format; + o->stride = stride; + return MP_OBJ_FROM_PTR(o); } @@ -851,24 +868,11 @@ STATIC MP_DEFINE_CONST_OBJ_TYPE( ); #endif -// this factory function is provided for backwards compatibility with old FrameBuffer1 class +// This factory function is provided for backwards compatibility with the old +// FrameBuffer1 class which did not support a format argument. STATIC mp_obj_t legacy_framebuffer1(size_t n_args, const mp_obj_t *args_in) { - mp_obj_framebuf_t *o = mp_obj_malloc(mp_obj_framebuf_t, (mp_obj_type_t *)&mp_type_framebuf); - - mp_buffer_info_t bufinfo; - mp_get_buffer_raise(args_in[0], &bufinfo, MP_BUFFER_WRITE); - o->buf = bufinfo.buf; - - o->width = mp_obj_get_int(args_in[1]); - o->height = mp_obj_get_int(args_in[2]); - o->format = FRAMEBUF_MVLSB; - if (n_args >= 4) { - o->stride = mp_obj_get_int(args_in[3]); - } else { - o->stride = o->width; - } - - return MP_OBJ_FROM_PTR(o); + mp_obj_t args[] = {args_in[0], args_in[1], args_in[2], MP_OBJ_NEW_SMALL_INT(FRAMEBUF_MVLSB), n_args >= 4 ? args_in[3] : args_in[1] }; + return framebuf_make_new(&mp_type_framebuf, 5, 0, args); } STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(legacy_framebuffer1_obj, 3, 4, legacy_framebuffer1); diff --git a/tests/extmod/framebuf_bounds.py b/tests/extmod/framebuf_bounds.py new file mode 100644 index 0000000000..3035b365bb --- /dev/null +++ b/tests/extmod/framebuf_bounds.py @@ -0,0 +1,60 @@ +try: + import framebuf +except ImportError: + print("SKIP") + raise SystemExit + + +def test_constructor(*args): + try: + framebuf.FrameBuffer(*args) + print("Valid") + except ValueError: + print("ValueError") + + +print(framebuf.MONO_HLSB) +test_constructor(bytearray(20), 10, 10, framebuf.MONO_HLSB) +test_constructor(bytearray(20), -1, 10, framebuf.MONO_HLSB) +test_constructor(bytearray(20), 10, -1, framebuf.MONO_HLSB) +test_constructor(bytearray(20), 10, 10, framebuf.MONO_HLSB, 11) +test_constructor(bytearray(20), 10, 10, framebuf.MONO_HLSB, 10) +test_constructor(bytearray(20), 10, 10, framebuf.MONO_HLSB, 9) +test_constructor(bytearray(20), 10, 10, framebuf.MONO_HLSB, -1) + +print(framebuf.MONO_VLSB) +test_constructor(bytearray(8), 8, 1, framebuf.MONO_VLSB) +test_constructor(bytearray(7), 8, 1, framebuf.MONO_VLSB) +test_constructor(bytearray(8), 8, 8, framebuf.MONO_VLSB) + +for f in (framebuf.MONO_HLSB, framebuf.MONO_HMSB): + print(f) + test_constructor(bytearray(1), 8, 1, f) + test_constructor(bytearray(0), 8, 1, f) + test_constructor(bytearray(8), 8, 8, f) + test_constructor(bytearray(9), 8, 9, f) + test_constructor(bytearray(9), 9, 8, f) + +print(framebuf.GS2_HMSB) +test_constructor(bytearray(8), 4, 8, framebuf.GS2_HMSB) +test_constructor(bytearray(15), 5, 8, framebuf.GS2_HMSB) +test_constructor(bytearray(16), 5, 8, framebuf.GS2_HMSB) +test_constructor(bytearray(9), 4, 9, framebuf.GS2_HMSB) + +print(framebuf.GS4_HMSB) +test_constructor(bytearray(8), 2, 8, framebuf.GS4_HMSB) +test_constructor(bytearray(15), 3, 8, framebuf.GS4_HMSB) +test_constructor(bytearray(16), 3, 8, framebuf.GS4_HMSB) +test_constructor(bytearray(9), 2, 9, framebuf.GS4_HMSB) + +print(framebuf.GS8) +test_constructor(bytearray(63), 8, 8, framebuf.GS8) +test_constructor(bytearray(64), 8, 8, framebuf.GS8) +test_constructor(bytearray(64), 9, 8, framebuf.GS8) +test_constructor(bytearray(64), 8, 9, framebuf.GS8) + +print(framebuf.RGB565) +test_constructor(bytearray(127), 8, 8, framebuf.RGB565) +test_constructor(bytearray(128), 8, 8, framebuf.RGB565) +test_constructor(bytearray(128), 9, 8, framebuf.RGB565) +test_constructor(bytearray(128), 8, 9, framebuf.RGB565) diff --git a/tests/extmod/framebuf_bounds.py.exp b/tests/extmod/framebuf_bounds.py.exp new file mode 100644 index 0000000000..3125013bc9 --- /dev/null +++ b/tests/extmod/framebuf_bounds.py.exp @@ -0,0 +1,44 @@ +3 +Valid +ValueError +ValueError +Valid +Valid +ValueError +ValueError +0 +Valid +ValueError +Valid +3 +Valid +ValueError +Valid +Valid +ValueError +4 +Valid +ValueError +Valid +Valid +ValueError +5 +Valid +ValueError +Valid +Valid +2 +Valid +ValueError +Valid +Valid +6 +ValueError +Valid +ValueError +ValueError +1 +ValueError +Valid +ValueError +ValueError