diff mbox series

[FFmpeg-devel,9/9] checkasm/hevc_pel: Fix stack buffer overreads

Message ID AM7PR03MB6660BC135A4D4AFF27CEB77A8FA89@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 09408539f4ad5d79f437d9b4789c5e64ad9b424f
Headers show
Series [FFmpeg-devel,1/9] avcodec/magicyuvenc: Use immediate when known | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 28, 2021, 2:41 p.m. UTC 
This patch increases several stack buffers in order to fix
stack-buffer-overflows (e.g. in put_hevc_qpel_uni_hv_9 in
line 814 of hevcdsp_template.c) detected with ASAN in the hevc_pel
checkasm test.
The buffers are increased by the minimal amount necessary
in order not to mask potential future bugs.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 tests/checkasm/hevc_pel.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Martin Storsjö Sept. 28, 2021, 7:36 p.m. UTC | #1 
On Tue, 28 Sep 2021, Andreas Rheinhardt wrote:

> This patch increases several stack buffers in order to fix
> stack-buffer-overflows (e.g. in put_hevc_qpel_uni_hv_9 in
> line 814 of hevcdsp_template.c) detected with ASAN in the hevc_pel
> checkasm test.
> The buffers are increased by the minimal amount necessary
> in order not to mask potential future bugs.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> tests/checkasm/hevc_pel.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)

Looks ok to me, thanks!

// Martin
Zhao Zhili Sept. 29, 2021, 2:19 a.m. UTC | #2 
> On Sep 28, 2021, at 10:41 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> This patch increases several stack buffers in order to fix
> stack-buffer-overflows (e.g. in put_hevc_qpel_uni_hv_9 in
> line 814 of hevcdsp_template.c) detected with ASAN in the hevc_pel
> checkasm test.
> The buffers are increased by the minimal amount necessary
> in order not to mask potential future bugs.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> tests/checkasm/hevc_pel.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
> 

LGTM, too.
diff mbox series

Patch

diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
index ec24309081..43aa5cd084 100644
--- a/tests/checkasm/hevc_pel.c
+++ b/tests/checkasm/hevc_pel.c
@@ -40,10 +40,12 @@  static const int offsets[] = {0, 255, -1 };
     do {                                             \
         uint32_t mask = pixel_mask[bit_depth - 8];   \
         int k;                                       \
-        for (k = 0; k < BUF_SIZE; k += 4) {          \
+        for (k = 0; k < BUF_SIZE + SRC_EXTRA; k += 4) { \
             uint32_t r = rnd() & mask;               \
             AV_WN32A(buf0 + k, r);                   \
             AV_WN32A(buf1 + k, r);                   \
+            if (k >= BUF_SIZE)                       \
+                continue;                            \
             r = rnd();                               \
             AV_WN32A(dst0 + k, r);                   \
             AV_WN32A(dst1 + k, r);                   \
@@ -65,10 +67,13 @@  static const int offsets[] = {0, 255, -1 };
 #define src0 (buf0 + 2 * 4 * MAX_PB_SIZE) /* hevc qpel functions read data from negative src pointer offsets */
 #define src1 (buf1 + 2 * 4 * MAX_PB_SIZE)
 
+/* FIXME: Does the need for SRC_EXTRA for these tests indicate a bug? */
+#define SRC_EXTRA 8
+
 static void checkasm_check_hevc_qpel(void)
 {
-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE + SRC_EXTRA]);
+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE + SRC_EXTRA]);
     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
 
@@ -111,8 +116,8 @@  static void checkasm_check_hevc_qpel(void)
 
 static void checkasm_check_hevc_qpel_uni(void)
 {
-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE + SRC_EXTRA]);
+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE + SRC_EXTRA]);
     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
 
@@ -152,8 +157,8 @@  static void checkasm_check_hevc_qpel_uni(void)
 
 static void checkasm_check_hevc_qpel_uni_w(void)
 {
-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE + SRC_EXTRA]);
+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE + SRC_EXTRA]);
     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
 
@@ -200,8 +205,8 @@  static void checkasm_check_hevc_qpel_uni_w(void)
 
 static void checkasm_check_hevc_qpel_bi(void)
 {
-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE + SRC_EXTRA]);
+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE + SRC_EXTRA]);
     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
     LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);
@@ -244,8 +249,8 @@  static void checkasm_check_hevc_qpel_bi(void)
 
 static void checkasm_check_hevc_qpel_bi_w(void)
 {
-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE + SRC_EXTRA]);
+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE + SRC_EXTRA]);
     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
     LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);
@@ -294,6 +299,9 @@  static void checkasm_check_hevc_qpel_bi_w(void)
     report("qpel_bi_w");
 }
 
+#undef SRC_EXTRA
+#define SRC_EXTRA 0
+
 static void checkasm_check_hevc_epel(void)
 {
     LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);