2023-07-26 07:47:17

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH 1/5] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

Always free the zeroed page on return from 'mwifiex_histogram_read()'.

Fixes: cbf6e05527a7 ("mwifiex: add rx histogram statistics support")
Signed-off-by: Dmitry Antipov <[email protected]>
---
NOTE: this series supersedes all of the previous mwifiex patches not
yet accepted into wireless-next tree.
---
drivers/net/wireless/marvell/mwifiex/debugfs.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index 52b18f4a774b..0cdd6c50c1c0 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -253,8 +253,11 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
if (!p)
return -ENOMEM;

- if (!priv || !priv->hist_data)
- return -EFAULT;
+ if (!priv || !priv->hist_data) {
+ ret = -EFAULT;
+ goto free_and_exit;
+ }
+
phist_data = priv->hist_data;

p += sprintf(p, "\n"
@@ -309,6 +312,8 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
ret = simple_read_from_buffer(ubuf, count, ppos, (char *)page,
(unsigned long)p - page);

+free_and_exit:
+ free_page(page);
return ret;
}

--
2.41.0



2023-07-26 07:49:01

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH 4/5] wifi: mwifiex: handle possible sscanf() errors

Return -EINVAL on possible 'sscanf()' errors in
'mwifiex_regrdwr_write()' and 'mwifiex_rdeeprom_write()'.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index 0cdd6c50c1c0..f9c9fec7c792 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -425,7 +425,10 @@ mwifiex_regrdwr_write(struct file *file,
if (IS_ERR(buf))
return PTR_ERR(buf);

- sscanf(buf, "%u %x %x", &reg_type, &reg_offset, &reg_value);
+ if (sscanf(buf, "%u %x %x", &reg_type, &reg_offset, &reg_value) != 3) {
+ ret = -EINVAL;
+ goto done;
+ }

if (reg_type == 0 || reg_offset == 0) {
ret = -EINVAL;
@@ -691,7 +694,10 @@ mwifiex_rdeeprom_write(struct file *file,
if (IS_ERR(buf))
return PTR_ERR(buf);

- sscanf(buf, "%d %d", &offset, &bytes);
+ if (sscanf(buf, "%d %d", &offset, &bytes) != 2) {
+ ret = -EINVAL;
+ goto done;
+ }

if (offset == -1 || bytes == -1) {
ret = -EINVAL;
--
2.41.0


2023-07-26 20:09:40

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/5] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

On Wed, Jul 26, 2023 at 10:20:52AM +0300, Dmitry Antipov wrote:
> Always free the zeroed page on return from 'mwifiex_histogram_read()'.
>
> Fixes: cbf6e05527a7 ("mwifiex: add rx histogram statistics support")
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> NOTE: this series supersedes all of the previous mwifiex patches not
> yet accepted into wireless-next tree.

I had comments for patch 2. Patch 1, 3, 4, 5 look good:

Acked-by: Brian Norris <[email protected]>

2023-07-28 09:07:00

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH 1/5] [v2] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

Always free the zeroed page on return from 'mwifiex_histogram_read()'.

Fixes: cbf6e05527a7 ("mwifiex: add rx histogram statistics support")
Signed-off-by: Dmitry Antipov <[email protected]>
---
v2: adjust to match series
---
drivers/net/wireless/marvell/mwifiex/debugfs.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index 52b18f4a774b..0cdd6c50c1c0 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -253,8 +253,11 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
if (!p)
return -ENOMEM;

- if (!priv || !priv->hist_data)
- return -EFAULT;
+ if (!priv || !priv->hist_data) {
+ ret = -EFAULT;
+ goto free_and_exit;
+ }
+
phist_data = priv->hist_data;

p += sprintf(p, "\n"
@@ -309,6 +312,8 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
ret = simple_read_from_buffer(ubuf, count, ppos, (char *)page,
(unsigned long)p - page);

+free_and_exit:
+ free_page(page);
return ret;
}

--
2.41.0


2023-07-28 09:12:20

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH 5/5] [v2] wifi: mwifiex: handle possible mwifiex_write_reg() errors

Return -1 on possible 'mwifiex_write_reg()' errors in
'mwifiex_init_sdio_ioport()', do not ignore the value
returned by the latter in 'mwifiex_init_sdio()' and
'mwifiex_sdio_up_dev()' as well.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <[email protected]>
---
v2: adjust to match series
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 22 +++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index a24bd40dd41a..0d60484cd5df 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -1083,17 +1083,17 @@ static int mwifiex_init_sdio_ioport(struct mwifiex_adapter *adapter)
"info: SDIO FUNC1 IO port: %#x\n", adapter->ioport);

/* Set Host interrupt reset to read to clear */
- if (!mwifiex_read_reg(adapter, card->reg->host_int_rsr_reg, &reg))
- mwifiex_write_reg(adapter, card->reg->host_int_rsr_reg,
- reg | card->reg->sdio_int_mask);
- else
+ if (mwifiex_read_reg(adapter, card->reg->host_int_rsr_reg, &reg))
+ return -1;
+ if (mwifiex_write_reg(adapter, card->reg->host_int_rsr_reg,
+ reg | card->reg->sdio_int_mask))
return -1;

/* Dnld/Upld ready set to auto reset */
- if (!mwifiex_read_reg(adapter, card->reg->card_misc_cfg_reg, &reg))
- mwifiex_write_reg(adapter, card->reg->card_misc_cfg_reg,
- reg | AUTO_RE_ENABLE_INT);
- else
+ if (mwifiex_read_reg(adapter, card->reg->card_misc_cfg_reg, &reg))
+ return -1;
+ if (mwifiex_write_reg(adapter, card->reg->card_misc_cfg_reg,
+ reg | AUTO_RE_ENABLE_INT))
return -1;

return 0;
@@ -2525,7 +2525,8 @@ static int mwifiex_init_sdio(struct mwifiex_adapter *adapter)
mwifiex_read_reg(adapter, card->reg->host_int_status_reg, &sdio_ireg);

/* Get SDIO ioport */
- mwifiex_init_sdio_ioport(adapter);
+ if (mwifiex_init_sdio_ioport(adapter))
+ return -EIO;

/* Initialize SDIO variables in card */
card->mp_rd_bitmap = 0;
@@ -3141,7 +3142,8 @@ static void mwifiex_sdio_up_dev(struct mwifiex_adapter *adapter)
*/
mwifiex_read_reg(adapter, card->reg->host_int_status_reg, &sdio_ireg);

- mwifiex_init_sdio_ioport(adapter);
+ if (mwifiex_init_sdio_ioport(adapter))
+ dev_err(&card->func->dev, "error enabling SDIO port\n");
}

static struct mwifiex_if_ops sdio_ops = {
--
2.41.0


2023-07-28 10:03:17

by Antipov, Dmitriy

[permalink] [raw]
Subject: Re: [lvc-project] [PATCH 1/5] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

On Wed, 2023-07-26 at 12:24 -0700, Brian Norris wrote:


> I had comments for patch 2. Patch 1, 3, 4, 5 look good:
>
> Acked-by: Brian Norris <[email protected]>

Should I add Acked-by: <you> to all of the above in case
of resend without changes, or leave it to the maintainer?

Dmitry

2023-07-31 10:00:22

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/5] [v2] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

Dmitry Antipov <[email protected]> writes:

> v2: adjust to match series

I don't know what that means. Please try to be specific in the changelog
entries. Also it might be easier for you if you include a cover letter
and place the changelog there.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2023-07-31 10:26:04

by Kalle Valo

[permalink] [raw]
Subject: Re: [lvc-project] [PATCH 1/5] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

"Antipov, Dmitriy" <[email protected]> writes:

> On Wed, 2023-07-26 at 12:24 -0700, Brian Norris wrote:
>
>
>> I had comments for patch 2. Patch 1, 3, 4, 5 look good:
>>
>> Acked-by: Brian Norris <[email protected]>
>
> Should I add Acked-by: <you> to all of the above in case
> of resend without changes, or leave it to the maintainer?

Adding Brian's Acked-by to patches 1, 3, 4, 5 is a good idea as long as
you don't change those patches. But no need to resend because of this.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2023-07-31 10:54:33

by Antipov, Dmitriy

[permalink] [raw]
Subject: Re: [lvc-project] [PATCH 1/5] [v2] wifi: mwifiex: fix memory leak in mwifiex_histogram_read()

On Mon, 2023-07-31 at 12:46 +0300, Kalle Valo wrote:

> > v2: adjust to match series
>
> I don't know what that means. Please try to be specific in the
> changelog entries.

Usually it means that if something is changed in the middle of the
series, surrounding patches are slightly tweaked to ensure that
everything still applies clearly (i.e. without offsets). If there
is something more substantial, I'm doing my best to explicitly
mention it.

Dmitry