staging: bcm2835-audio: Fix memory corruption

PATCH In review Phil Elwell 2 participants 8 messages Aug 8, 2017 – Aug 10, 2017

Subsystems (3)

Staging Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture

lore

Share & tools

lore

Phil Elwell

author

2017-08-08 12:11 UTC #0 lore raw

PATCH 1/1 staging: bcm2835-audio: Fix memory corruption

I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.

1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.

Signed-off-by Phil Elwell <[email protected]>

Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1

index 5f3d8f2..89f96f3 100644
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 
+			vchi_disconnect(vchi_instance);
 			ret = -EIO;
 			goto err_free_mem;
 		}

@@ -431,7 +432,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	LOG_DBG(" success !\n");
 	ret = 0;
 err_free_mem:
-	kfree(vchi_instance);
 
 	return ret;
 }

Signature · 1 line

1.9.1

Dan Carpenter

2017-08-10 10:25 UTC #1 lore raw

The original patch did not go through the normal review process...

Quoted 3 lines · Phil Elwell

I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.

This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.

Quoted 14 lines

1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412

Quoted 4 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0

@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 
+			vchi_disconnect(vchi_instance);

This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 414

Quoted 2 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
 			ret = -EIO;
 			goto err_free_mem;

This label name is out of date. There is a later error path where vc_vchi_audio_init() fails and we leak on that path. Also why is vchi_instance a static variable? Arglebargleblargleblah...

regards,
dan carpenter

Phil Elwell

author

2017-08-10 10:52 UTC #2 lore raw

Quoted 9 lines · Dan Carpenter

The original patch did not go through the normal review process...
Quoted 3 lines · Phil Elwell
I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.
This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.

It's more subtle than that - the failure isn't consistent, sometimes crashing and sometimes not depending on how and when memory is reused.

Quoted 17 lines

Quoted 17 lines
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 

Quoted 5 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412
Quoted 1 line
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0
+			vchi_disconnect(vchi_instance);
This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().

Agreed - I'm not going to change the API, but I can add a comment.

Quoted 5 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 414
Quoted 2 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
 			ret = -EIO;
 			goto err_free_mem;
This label name is out of date. There is a later error path where vc_vchi_audio_init() fails and we leak on that path.

Also agreed. I'll rework it.

Quoted 2 lines

Also why is
vchi_instance a static variable? Arglebargleblargleblah...

You may well think that. I couldn't possibly comment.

Phil

Dan Carpenter

2017-08-10 11:06 UTC #3 lore raw

Quoted 5 lines · Phil Elwell

Quoted 2 lines
This label name is out of date. There is a later error path where vc_vchi_audio_init() fails and we leak on that path.
Also agreed. I'll rework it.

Actually I wasn't right. That error path should probably stay how it is, because we're re-using the vchi_instance. We allocate it in the first call then re-use it later.

This code is really subtle and ugly.

regards,
dan carpenter

Dan Carpenter

2017-08-10 11:25 UTC #4 lore raw

In reply to Phil Elwell

Quoted 31 lines · Phil Elwell

Quoted 9 lines · Dan Carpenter
The original patch did not go through the normal review process...
Quoted 3 lines · Phil Elwell
I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.
This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.
It's more subtle than that - the failure isn't consistent, sometimes crashing and sometimes not depending on how and when memory is reused.
Quoted 17 lines
Quoted 17 lines
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 

Quoted 8 lines

Quoted 5 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412
Quoted 1 line
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0
+			vchi_disconnect(vchi_instance);
This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().
Agreed - I'm not going to change the API, but I can add a comment.

Nah... Please don't do that. Just create a function:

static void vchiq_free(VCHIQ_INSTANCE_T *vchi_instance)

{
	kfree(vchi_instance);
}

Really vchi_initialise() is badly named and it's just a wrapper around vchiq_initialise(). It should be deleted and vchiq_initialise() should be renamed to allocate instead of initialize... But that's for a separate patch.

And also we should move the kfree() out of disconnect() like I said and instead call vchiq_free(). But again, that's for a separate patch.

Change the goto to "return -EIO." Leave the last error path as-is. Eventually we will want to break this into two functions, one that allocates the first vchi_instance and one that calls vc_vchi_audio_init(). But again, there are many patches needed to fix this code.

regards,
dan carpenter

Dan Carpenter

2017-08-10 11:35 UTC #5 lore raw

Quoted 38 lines · Dan Carpenter

Quoted 37 lines · Phil Elwell
Quoted 9 lines · Dan Carpenter
The original patch did not go through the normal review process...
Quoted 3 lines · Phil Elwell
I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.
This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.
It's more subtle than that - the failure isn't consistent, sometimes crashing and sometimes not depending on how and when memory is reused.
Quoted 22 lines
Quoted 14 lines
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412
Quoted 4 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 
+			vchi_disconnect(vchi_instance);
This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().

Quoted 6 lines

Quoted 2 lines
Agreed - I'm not going to change the API, but I can add a comment.
Nah... Please don't do that. Just create a function:
static void vchiq_free(VCHIQ_INSTANCE_T *vchi_instance)

^^^^^^
Not static. My bad.

regards,
dan carpenter

Phil Elwell

author

2017-08-10 13:26 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 53 lines · Dan Carpenter

Quoted 31 lines · Phil Elwell
Quoted 9 lines · Dan Carpenter
The original patch did not go through the normal review process...
Quoted 3 lines · Phil Elwell
I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.
This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.
It's more subtle than that - the failure isn't consistent, sometimes crashing and sometimes not depending on how and when memory is reused.
Quoted 17 lines
Quoted 17 lines
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 
Quoted 8 lines
Quoted 5 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412
Quoted 1 line
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0
+			vchi_disconnect(vchi_instance);
This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().
Agreed - I'm not going to change the API, but I can add a comment.
Nah... Please don't do that. Just create a function:
void vchiq_free(VCHIQ_INSTANCE_T *vchi_instance)
{
	kfree(vchi_instance);
}
Really vchi_initialise() is badly named and it's just a wrapper around vchiq_initialise(). It should be deleted and vchiq_initialise() should be renamed to allocate instead of initialize... But that's for a separate patch.

Quoted 7 lines

And also we should move the kfree() out of disconnect() like I said and instead call vchiq_free(). But again, that's for a separate patch.
Change the goto to "return -EIO." Leave the last error path as-is. Eventually we will want to break this into two functions, one that allocates the first vchi_instance and one that calls vc_vchi_audio_init(). But again, there are many patches needed to fix this code.

[ static removed ]

Shouldn't that be:

void vchi_uninitialise(VCHI_INSTANCE_T instance_handle)

{
	vchiq_shutdown((VCHIQ_INSTANCE_T)instance_handle);
}

EXPORT_SYMBOL(vchi_uninitialise);

Yes, in this case the instance is just a block of memory with no users, but if this is a general API call then you want it to work in all cases, and calling vchiq_shutdown is the right way to free an instance. Calling a VCHIQ method when all the other calls are to the VCHI API is grubby, so make it a VCHI method instead.

If instead this is just another hack - a kfree with a comment in code form - then lets keep it static within the audio driver (which is what I thought you were initially suggesting).

Phil

Dan Carpenter

2017-08-10 14:08 UTC #7 lore raw

Quoted 64 lines · Phil Elwell

Quoted 53 lines · Dan Carpenter
Quoted 31 lines · Phil Elwell
Quoted 9 lines · Dan Carpenter
The original patch did not go through the normal review process...
Quoted 3 lines · Phil Elwell
I'm all for fixing memory leaks, but freeing a block while it is still being used is a recipe for hard-to-debug kernel exceptions.
This bug completely breaks the driver doesn't it? It's not very subtle so it should be easy to diagnose with git bisect.
It's more subtle than that - the failure isn't consistent, sometimes crashing and sometimes not depending on how and when memory is reused.
Quoted 17 lines
Quoted 17 lines
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by Phil Elwell <[email protected]>
Fixes 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()")
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -0
index 5f3d8f2..89f96f3 100644
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
 				__func__, ret);
 
Quoted 8 lines
Quoted 5 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream · line 412
Quoted 1 line
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -0
+			vchi_disconnect(vchi_instance);
This is ugly because why are we calling disconnect() if connect() fails? These functions should be symetric so disconnect only disconnects and we call a different function to undo vchi_initialise().
Agreed - I'm not going to change the API, but I can add a comment.
Nah... Please don't do that. Just create a function:
void vchiq_free(VCHIQ_INSTANCE_T *vchi_instance)
{
	kfree(vchi_instance);
}
Really vchi_initialise() is badly named and it's just a wrapper around vchiq_initialise(). It should be deleted and vchiq_initialise() should be renamed to allocate instead of initialize... But that's for a separate patch.
Quoted 7 lines
And also we should move the kfree() out of disconnect() like I said and instead call vchiq_free(). But again, that's for a separate patch.
Change the goto to "return -EIO." Leave the last error path as-is. Eventually we will want to break this into two functions, one that allocates the first vchi_instance and one that calls vc_vchi_audio_init(). But again, there are many patches needed to fix this code.

Quoted 5 lines

[ static removed ]
Shouldn't that be:
void vchi_uninitialise(VCHI_INSTANCE_T instance_handle)

I haven't understood the difference between vchi_ and vchiq_. It looks like vhci_ is just a shim around vhciq_. We should get rid of the shim layer eventually...

Naming it vhci_ would make it consistent with the rest of the shim layer but we don't care about consistency much we just want to write the code the way it will end up eventually. In other words, there is no point creating a shim function just to be consistent.

initialize/uninitialize are bad names because really it's an alloc/free.

Anyway, I don't have strong feelings about the name.

Quoted 10 lines

{
	vchiq_shutdown((VCHIQ_INSTANCE_T)instance_handle);
}
EXPORT_SYMBOL(vchi_uninitialise);
Yes, in this case the instance is just a block of memory with no users, but if this is a general API call then you want it to work in all cases, and calling vchiq_shutdown is the right way to free an instance. Calling a VCHIQ method when all the other calls are to the VCHI API is grubby, so make it a VCHI method instead.

The vchiq_shutdown() does call kfree() but it's a layering violation. Every allocation function should have a mirror free function. The vchiq_shutdown() cleans up a bunch of stuff which was not allocated in vchi_initialise() and it seems to all be a no-op so it's not buggy but it's not the right way to design an API.

So: Rename vchiq_shutdown() to vchiq_disconnect(). Delete the kfree(). Create a new function vchiq_shutdown() that does:

void vchiq_shutdown()

{
	vchiq_disconnect();
	vchiq_free();
}

But again, that is work for future patches. Every alloc has a free, every connect has a disconnect.

regards,
dan carpenter