On 11/12/24 21:28, Suraj Sonawane wrote:
Fix an error detected by the Smatch tool:
drivers/video/fbdev/metronomefb.c:220 load_waveform() error:
buffer overflow 'wfm_hdr->stuff2a' 2 <= 4
drivers/video/fbdev/metronomefb.c:220 load_waveform() error:
buffer overflow 'wfm_hdr->stuff2a' 2 <= 4
The access to wfm_hdr->stuff2a in the loop can lead to a buffer
overflow if stuff2a is not large enough. To fix this, a check was
added to ensure that stuff2a has sufficient space before accessing
it. This prevents the overflow and improves the safety of the code.
Signed-off-by: Suraj Sonawane <surajsonawane0215@xxxxxxxxx>
---
drivers/video/fbdev/metronomefb.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 6f0942c6e..9da55cef2 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -210,6 +210,12 @@ static int load_waveform(u8 *mem, size_t size, int m, int t,
}
wfm_hdr->mc += 1;
wfm_hdr->trc += 1;
+
+ if (sizeof(wfm_hdr->stuff2a) < 5) {
+ dev_err(dev, "Error: insufficient space in stuff2a\n");
+ return -EINVAL;
+ }
+
for (i = 0; i < 5; i++) {
if (*(wfm_hdr->stuff2a + i) != 0) {
dev_err(dev, "Error: unexpected value in padding\n");
That patch is completely wrong.
There is
/* the waveform structure that is coming from userspace firmware */
struct waveform_hdr {
....
u8 stuff2a[2];
u8 stuff2b[3];
So, I *believe* the for-next loop wants to walk acrosss stuff2a and stuff2b,
which have 5 entries together. So, basically the original code isn't nice
but still correct.
Your "sizeof()" check will always be false and is the wrong patch.
If at all, I think the stuff2a and stuff 2b arrays should be joined.
Something like
u8 stuff2[5]; /* this is actually 2-entry stuff2a and 3-entry stuff2b */
But again, I don't know much about this driver.
Helge
