| Message ID | 20241112202825.17322-1-surajsonawane0215@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | video: fbdev: metronomefb: Fix buffer overflow in load_waveform() | expand |
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@gmail.com> > --- > 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
On 14/11/24 20:13, Helge Deller wrote: > 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@gmail.com> >> --- >> 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 Thank you for the brief feedback. I see your point regarding stuff2a and stuff2b. I’ll study this approach and revise the patch if I find it to be the correct solution. Best regards, Suraj Sonawane
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");
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@gmail.com> --- drivers/video/fbdev/metronomefb.c | 6 ++++++ 1 file changed, 6 insertions(+)