diff mbox series

wfx: prevent underflow in wfx_send_pds()

Message ID Yv8eX7Xv2ubUOvW7@kili
State New
Headers show
Series wfx: prevent underflow in wfx_send_pds() | expand

Commit Message

Dan Carpenter Aug. 19, 2022, 5:23 a.m. UTC 
This does a "chunk_len - 4" subtraction later when it calls:

	ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);

so check for "chunk_len" is less than 4.

Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/net/wireless/silabs/wfx/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jérôme Pouiller Aug. 22, 2022, 12:20 p.m. UTC | #1 
On Friday 19 August 2022 07:23:43 CEST Dan Carpenter wrote:
> This does a "chunk_len - 4" subtraction later when it calls:
> 
>         ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
> 
> so check for "chunk_len" is less than 4.

This patch also ensures that buf[4] won't overflow during:

    if (buf[4] != '{' || buf[chunk_len - 1] != '}')
    	dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);

> Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
Kalle Valo Aug. 29, 2022, 4:03 p.m. UTC | #2 
Jérôme Pouiller <jerome.pouiller@silabs.com> writes:

> On Friday 19 August 2022 07:23:43 CEST Dan Carpenter wrote:
>> This does a "chunk_len - 4" subtraction later when it calls:
>> 
>>         ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
>> 
>> so check for "chunk_len" is less than 4.
>
> This patch also ensures that buf[4] won't overflow during:
>
>     if (buf[4] != '{' || buf[chunk_len - 1] != '}')
>     	dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);
>
>> Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

BTW Jérôme, as you are the driver maintainer you can use Acked-by.
Jérôme Pouiller Aug. 29, 2022, 5:12 p.m. UTC | #3 
On Monday 29 August 2022 18:03:38 CEST Kalle Valo wrote:
> Jérôme Pouiller <jerome.pouiller@silabs.com> writes:
> 
> > On Friday 19 August 2022 07:23:43 CEST Dan Carpenter wrote:
> >> This does a "chunk_len - 4" subtraction later when it calls:
> >>
> >>         ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
> >>
> >> so check for "chunk_len" is less than 4.
> >
> > This patch also ensures that buf[4] won't overflow during:
> >
> >     if (buf[4] != '{' || buf[chunk_len - 1] != '}')
> >       dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);
> >
> >> Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> BTW Jérôme, as you are the driver maintainer you can use Acked-by.

Reviewed-by does not imply Acked-by?
Kalle Valo Sept. 1, 2022, 10:33 a.m. UTC | #4 
Jérôme Pouiller <jerome.pouiller@silabs.com> writes:

> On Monday 29 August 2022 18:03:38 CEST Kalle Valo wrote:
>> Jérôme Pouiller <jerome.pouiller@silabs.com> writes:
>> 
>> > On Friday 19 August 2022 07:23:43 CEST Dan Carpenter wrote:
>> >> This does a "chunk_len - 4" subtraction later when it calls:
>> >>
>> >>         ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
>> >>
>> >> so check for "chunk_len" is less than 4.
>> >
>> > This patch also ensures that buf[4] won't overflow during:
>> >
>> >     if (buf[4] != '{' || buf[chunk_len - 1] != '}')
>> >       dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);
>> >
>> >> Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
>> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> >
>> > Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
>> 
>> BTW Jérôme, as you are the driver maintainer you can use Acked-by.
>
> Reviewed-by does not imply Acked-by?

Acked-by has "stronger" meaning and is meant to use by the maintainer of
the code in question. So anyone can use Reviewed-by but only the
maintainer should use Acked-by. My preference is that maintainers use
Acked-by as then I can easily see from my patchwork script that the
patch is ready to be applied.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Kalle Valo Sept. 2, 2022, 8:44 a.m. UTC | #5 
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> This does a "chunk_len - 4" subtraction later when it calls:
> 
> 	ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
> 
> so check for "chunk_len" is less than 4.
> 
> Fixes: dcbecb497908 ("staging: wfx: allow new PDS format")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

Patch applied to wireless-next.git, thanks.

f97c81f5b7f8 wifi: wfx: prevent underflow in wfx_send_pds()
diff mbox series

Patch

diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
index e015bfb8d221..84d82ddded56 100644
--- a/drivers/net/wireless/silabs/wfx/main.c
+++ b/drivers/net/wireless/silabs/wfx/main.c
@@ -181,7 +181,7 @@  int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
 	while (len > 0) {
 		chunk_type = get_unaligned_le16(buf + 0);
 		chunk_len = get_unaligned_le16(buf + 2);
-		if (chunk_len > len) {
+		if (chunk_len < 4 || chunk_len > len) {
 			dev_err(wdev->dev, "PDS:%d: corrupted file\n", chunk_num);
 			return -EINVAL;
 		}