[PATCH] video: fbdev: metronomefb: Fix buffer overflow in load_waveform()

[PATCH] video: fbdev: metronomefb: Fix buffer overflow in load_waveform()

[Suraj Sonawane]

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");
-- 
2.34.1

Re: [PATCH] video: fbdev: metronomefb: Fix buffer overflow in load_waveform()

[Helge Deller]

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

Re: [PATCH] video: fbdev: metronomefb: Fix buffer overflow in load_waveform()

[Suraj Sonawane]

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