2017-08-11 10:21:17

by Phil Elwell

Subject: [PATCH v2] staging: bcm2835-audio: Fix memory corruption

The previous commit (0adbfd46) fixed a memory leak but also freed a
block in the success case, causing a stale pointer to be used with
potentially fatal results. Only free the vchi_instance block in the
case that vchi_connect fails; once connected, the instance is
retained for subsequent connections.

Simplifying the code by removing a bunch of gotos and returning errors
directly.

Signed-off-by: Phil Elwell <[email protected]>
Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
[Resend with v2 in subject]
v2: Simplified following feedback from Dan Carpenter.
---
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 5f3d8f2..4be864d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -390,8 +390,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
__func__, instance);
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
- ret = 0; // xxx todo -1;
- goto err_free_mem;
+ return 0;
}

/* Initialize and create a VCHI connection */
@@ -401,16 +400,15 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
LOG_ERR("%s: failed to initialise VCHI instance (ret=%d)\n",
__func__, ret);

- ret = -EIO;
- goto err_free_mem;
+ return -EIO;
}
ret = vchi_connect(NULL, 0, vchi_instance);
if (ret) {
LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
__func__, ret);

- ret = -EIO;
- goto err_free_mem;
+ kfree(vchi_instance);
+ return -EIO;
}
initted = 1;
}
@@ -421,19 +419,16 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
if (IS_ERR(instance)) {
LOG_ERR("%s: failed to initialize audio service\n", __func__);

- ret = PTR_ERR(instance);
- goto err_free_mem;
+ /* vchi_instance is retained for use the next time. */
+ return PTR_ERR(instance);
}

instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;

LOG_DBG(" success !\n");
- ret = 0;
-err_free_mem:
- kfree(vchi_instance);

- return ret;
+ return 0;
}

int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
--
1.9.1

2017-08-11 13:47:24

by Dan Carpenter

Subject: Re: [PATCH v2] staging: bcm2835-audio: Fix memory corruption

Looks Ok to me.

regards,
dan carpenter

2017-09-23 10:58:20

by Stefan Wahren

Subject: Re: [PATCH v2] staging: bcm2835-audio: Fix memory corruption

Hi Greg,

> Phil Elwell <[email protected]> hat am 11. August 2017 um 12:20 geschrieben:
>
>
> The previous commit (0adbfd46) fixed a memory leak but also freed a
> block in the success case, causing a stale pointer to be used with
> potentially fatal results. Only free the vchi_instance block in the
> case that vchi_connect fails; once connected, the instance is
> retained for subsequent connections.
>
> Simplifying the code by removing a bunch of gotos and returning errors
> directly.
>
> Signed-off-by: Phil Elwell <[email protected]>
> Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")

can you still apply this patch or do you need a resend?

> ---
> [Resend with v2 in subject]
> v2: Simplified following feedback from Dan Carpenter.
> ---
> .../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index 5f3d8f2..4be864d 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -390,8 +390,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
> __func__, instance);
> instance->alsa_stream = alsa_stream;
> alsa_stream->instance = instance;
> - ret = 0; // xxx todo -1;
> - goto err_free_mem;
> + return 0;
> }
>
> /* Initialize and create a VCHI connection */
> @@ -401,16 +400,15 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
> LOG_ERR("%s: failed to initialise VCHI instance (ret=%d)\n",
> __func__, ret);
>
> - ret = -EIO;
> - goto err_free_mem;
> + return -EIO;
> }
> ret = vchi_connect(NULL, 0, vchi_instance);
> if (ret) {
> LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
> __func__, ret);
>
> - ret = -EIO;
> - goto err_free_mem;
> + kfree(vchi_instance);
> + return -EIO;
> }
> initted = 1;
> }
> @@ -421,19 +419,16 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
> if (IS_ERR(instance)) {
> LOG_ERR("%s: failed to initialize audio service\n", __func__);
>
> - ret = PTR_ERR(instance);
> - goto err_free_mem;
> + /* vchi_instance is retained for use the next time. */
> + return PTR_ERR(instance);
> }
>
> instance->alsa_stream = alsa_stream;
> alsa_stream->instance = instance;
>
> LOG_DBG(" success !\n");
> - ret = 0;
> -err_free_mem:
> - kfree(vchi_instance);
>
> - return ret;
> + return 0;
> }
>
> int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
> --
> 1.9.1
>

2017-09-23 16:15:45

by Greg Kroah-Hartman

Subject: Re: [PATCH v2] staging: bcm2835-audio: Fix memory corruption

On Sat, Sep 23, 2017 at 12:57:33PM +0200, Stefan Wahren wrote:
> Hi Greg,
>
> > Phil Elwell <[email protected]> hat am 11. August 2017 um 12:20 geschrieben:
> >
> >
> > The previous commit (0adbfd46) fixed a memory leak but also freed a
> > block in the success case, causing a stale pointer to be used with
> > potentially fatal results. Only free the vchi_instance block in the
> > case that vchi_connect fails; once connected, the instance is
> > retained for subsequent connections.
> >
> > Simplifying the code by removing a bunch of gotos and returning errors
> > directly.
> >
> > Signed-off-by: Phil Elwell <[email protected]>
> > Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
>
> can you still apply this patch or do you need a resend?

Hm, I don't see this anywhere in my tree, or in my todo mbox, so yes,
please resend it, thanks.

greg k-h

2017-09-23 22:45:26

by Stefan Wahren

Subject: Re: [PATCH v2] staging: bcm2835-audio: Fix memory corruption

Hi Phil,

> Greg Kroah-Hartman <[email protected]> hat am 23. September 2017 um 18:15 geschrieben:
>
>
> On Sat, Sep 23, 2017 at 12:57:33PM +0200, Stefan Wahren wrote:
> > Hi Greg,
> >
> > > Phil Elwell <[email protected]> hat am 11. August 2017 um 12:20 geschrieben:
> > >
> > >
> > > The previous commit (0adbfd46) fixed a memory leak but also freed a
> > > block in the success case, causing a stale pointer to be used with
> > > potentially fatal results. Only free the vchi_instance block in the
> > > case that vchi_connect fails; once connected, the instance is
> > > retained for subsequent connections.
> > >
> > > Simplifying the code by removing a bunch of gotos and returning errors
> > > directly.
> > >
> > > Signed-off-by: Phil Elwell <[email protected]>
> > > Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
> >
> > can you still apply this patch or do you need a resend?
>
> Hm, I don't see this anywhere in my tree, or in my todo mbox, so yes,
> please resend it, thanks.

could you please resend this patch?

Thanks
Stefan

>
> greg k-h