All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
@ 2019-08-15  8:30 ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-08-15  8:30 UTC (permalink / raw)
  To: VMware Graphics, Colin Ian King
  Cc: Thomas Hellstrom, David Airlie, Daniel Vetter, dri-devel,
	linux-kernel, kernel-janitors

We recently added a kfree() after the end of the loop:

	if (retries == RETRIES) {
		kfree(reply);
		return -EINVAL;
	}

There are two problems.  First the test is wrong and because retries
equals RETRIES if we succeed on the last iteration through the loop.
Second if we fail on the last iteration through the loop then the kfree
is a double free.

When you're reading this code, please note the break statement at the
end of the while loop.  This patch changes the loop so that if it's not
successful then "reply" is NULL and we can test for that afterward.

Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many retries have occurred")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index 59e9d05ab928..0af048d1a815 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 				     !!(HIGH_WORD(ecx) & MESSAGE_STATUS_HB));
 		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
 			kfree(reply);
-
+			reply = NULL;
 			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
 				/* A checkpoint occurred. Retry. */
 				continue;
@@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 
 		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
 			kfree(reply);
-
+			reply = NULL;
 			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
 				/* A checkpoint occurred. Retry. */
 				continue;
@@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 		break;
 	}
 
-	if (retries == RETRIES) {
-		kfree(reply);
+	if (!reply)
 		return -EINVAL;
-	}
 
 	*msg_len = reply_len;
 	*msg     = reply;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
@ 2019-08-15  8:30 ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-08-15  8:30 UTC (permalink / raw)
  To: VMware Graphics, Colin Ian King
  Cc: Thomas Hellstrom, David Airlie, Daniel Vetter, dri-devel,
	linux-kernel, kernel-janitors

We recently added a kfree() after the end of the loop:

	if (retries = RETRIES) {
		kfree(reply);
		return -EINVAL;
	}

There are two problems.  First the test is wrong and because retries
equals RETRIES if we succeed on the last iteration through the loop.
Second if we fail on the last iteration through the loop then the kfree
is a double free.

When you're reading this code, please note the break statement at the
end of the while loop.  This patch changes the loop so that if it's not
successful then "reply" is NULL and we can test for that afterward.

Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many retries have occurred")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index 59e9d05ab928..0af048d1a815 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 				     !!(HIGH_WORD(ecx) & MESSAGE_STATUS_HB));
 		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) = 0) {
 			kfree(reply);
-
+			reply = NULL;
 			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
 				/* A checkpoint occurred. Retry. */
 				continue;
@@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 
 		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) = 0) {
 			kfree(reply);
-
+			reply = NULL;
 			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
 				/* A checkpoint occurred. Retry. */
 				continue;
@@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
 		break;
 	}
 
-	if (retries = RETRIES) {
-		kfree(reply);
+	if (!reply)
 		return -EINVAL;
-	}
 
 	*msg_len = reply_len;
 	*msg     = reply;
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
  2019-08-15  8:30 ` Dan Carpenter
@ 2019-08-15  8:38   ` Colin Ian King
  -1 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2019-08-15  8:38 UTC (permalink / raw)
  To: Dan Carpenter, VMware Graphics
  Cc: Thomas Hellstrom, David Airlie, Daniel Vetter, dri-devel,
	linux-kernel, kernel-janitors

On 15/08/2019 09:30, Dan Carpenter wrote:
> We recently added a kfree() after the end of the loop:
> 
> 	if (retries == RETRIES) {
> 		kfree(reply);
> 		return -EINVAL;
> 	}
> 
> There are two problems.  First the test is wrong and because retries
> equals RETRIES if we succeed on the last iteration through the loop.
> Second if we fail on the last iteration through the loop then the kfree
> is a double free.
> 
> When you're reading this code, please note the break statement at the
> end of the while loop.  This patch changes the loop so that if it's not
> successful then "reply" is NULL and we can test for that afterward.
> 
> Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many retries have occurred")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> index 59e9d05ab928..0af048d1a815 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> @@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  				     !!(HIGH_WORD(ecx) & MESSAGE_STATUS_HB));
>  		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
>  			kfree(reply);
> -
> +			reply = NULL;
>  			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
>  				/* A checkpoint occurred. Retry. */
>  				continue;
> @@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  
>  		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
>  			kfree(reply);
> -
> +			reply = NULL;
>  			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
>  				/* A checkpoint occurred. Retry. */
>  				continue;
> @@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  		break;
>  	}
>  
> -	if (retries == RETRIES) {
> -		kfree(reply);
> +	if (!reply)
>  		return -EINVAL;
> -	}
>  
>  	*msg_len = reply_len;
>  	*msg     = reply;
> 

Dan, Thanks for fixing up my mistake.

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
@ 2019-08-15  8:38   ` Colin Ian King
  0 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2019-08-15  8:38 UTC (permalink / raw)
  To: Dan Carpenter, VMware Graphics
  Cc: Thomas Hellstrom, David Airlie, Daniel Vetter, dri-devel,
	linux-kernel, kernel-janitors

On 15/08/2019 09:30, Dan Carpenter wrote:
> We recently added a kfree() after the end of the loop:
> 
> 	if (retries = RETRIES) {
> 		kfree(reply);
> 		return -EINVAL;
> 	}
> 
> There are two problems.  First the test is wrong and because retries
> equals RETRIES if we succeed on the last iteration through the loop.
> Second if we fail on the last iteration through the loop then the kfree
> is a double free.
> 
> When you're reading this code, please note the break statement at the
> end of the while loop.  This patch changes the loop so that if it's not
> successful then "reply" is NULL and we can test for that afterward.
> 
> Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many retries have occurred")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> index 59e9d05ab928..0af048d1a815 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> @@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  				     !!(HIGH_WORD(ecx) & MESSAGE_STATUS_HB));
>  		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) = 0) {
>  			kfree(reply);
> -
> +			reply = NULL;
>  			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
>  				/* A checkpoint occurred. Retry. */
>  				continue;
> @@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  
>  		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) = 0) {
>  			kfree(reply);
> -
> +			reply = NULL;
>  			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
>  				/* A checkpoint occurred. Retry. */
>  				continue;
> @@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
>  		break;
>  	}
>  
> -	if (retries = RETRIES) {
> -		kfree(reply);
> +	if (!reply)
>  		return -EINVAL;
> -	}
>  
>  	*msg_len = reply_len;
>  	*msg     = reply;
> 

Dan, Thanks for fixing up my mistake.

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
  2019-08-15  8:38   ` Colin Ian King
  (?)
@ 2019-09-05 11:54     ` Thomas Hellstrom
  -1 siblings, 0 replies; 7+ messages in thread
From: Thomas Hellstrom @ 2019-09-05 11:54 UTC (permalink / raw)
  To: colin.king, dan.carpenter, Linux-graphics-maintainer
  Cc: daniel, dri-devel, linux-kernel, airlied, kernel-janitors

On Thu, 2019-08-15 at 09:38 +0100, Colin Ian King wrote:
> On 15/08/2019 09:30, Dan Carpenter wrote:
> > We recently added a kfree() after the end of the loop:
> > 
> > 	if (retries == RETRIES) {
> > 		kfree(reply);
> > 		return -EINVAL;
> > 	}
> > 
> > There are two problems.  First the test is wrong and because
> > retries
> > equals RETRIES if we succeed on the last iteration through the
> > loop.
> > Second if we fail on the last iteration through the loop then the
> > kfree
> > is a double free.
> > 
> > When you're reading this code, please note the break statement at
> > the
> > end of the while loop.  This patch changes the loop so that if it's
> > not
> > successful then "reply" is NULL and we can test for that afterward.
> > 
> > Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many
> > retries have occurred")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > index 59e9d05ab928..0af048d1a815 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > @@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  				     !!(HIGH_WORD(ecx) &
> > MESSAGE_STATUS_HB));
> >  		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
> >  			kfree(reply);
> > -
> > +			reply = NULL;
> >  			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0)
> > {
> >  				/* A checkpoint occurred. Retry. */
> >  				continue;
> > @@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  
> >  		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
> >  			kfree(reply);
> > -
> > +			reply = NULL;
> >  			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0)
> > {
> >  				/* A checkpoint occurred. Retry. */
> >  				continue;
> > @@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  		break;
> >  	}
> >  
> > -	if (retries == RETRIES) {
> > -		kfree(reply);
> > +	if (!reply)
> >  		return -EINVAL;
> > -	}
> >  
> >  	*msg_len = reply_len;
> >  	*msg     = reply;
> > 
> 
> Dan, Thanks for fixing up my mistake.

Thanks, Dan. Sorry for the late reply. 
Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Will push this to fixes.

/Thomas


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
@ 2019-09-05 11:54     ` Thomas Hellstrom
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Hellstrom @ 2019-09-05 11:54 UTC (permalink / raw)
  To: colin.king, dan.carpenter, Linux-graphics-maintainer
  Cc: airlied, kernel-janitors, dri-devel, linux-kernel

T24gVGh1LCAyMDE5LTA4LTE1IGF0IDA5OjM4ICswMTAwLCBDb2xpbiBJYW4gS2luZyB3cm90ZToN
Cj4gT24gMTUvMDgvMjAxOSAwOTozMCwgRGFuIENhcnBlbnRlciB3cm90ZToNCj4gPiBXZSByZWNl
bnRseSBhZGRlZCBhIGtmcmVlKCkgYWZ0ZXIgdGhlIGVuZCBvZiB0aGUgbG9vcDoNCj4gPiANCj4g
PiAJaWYgKHJldHJpZXMgPT0gUkVUUklFUykgew0KPiA+IAkJa2ZyZWUocmVwbHkpOw0KPiA+IAkJ
cmV0dXJuIC1FSU5WQUw7DQo+ID4gCX0NCj4gPiANCj4gPiBUaGVyZSBhcmUgdHdvIHByb2JsZW1z
LiAgRmlyc3QgdGhlIHRlc3QgaXMgd3JvbmcgYW5kIGJlY2F1c2UNCj4gPiByZXRyaWVzDQo+ID4g
ZXF1YWxzIFJFVFJJRVMgaWYgd2Ugc3VjY2VlZCBvbiB0aGUgbGFzdCBpdGVyYXRpb24gdGhyb3Vn
aCB0aGUNCj4gPiBsb29wLg0KPiA+IFNlY29uZCBpZiB3ZSBmYWlsIG9uIHRoZSBsYXN0IGl0ZXJh
dGlvbiB0aHJvdWdoIHRoZSBsb29wIHRoZW4gdGhlDQo+ID4ga2ZyZWUNCj4gPiBpcyBhIGRvdWJs
ZSBmcmVlLg0KPiA+IA0KPiA+IFdoZW4geW91J3JlIHJlYWRpbmcgdGhpcyBjb2RlLCBwbGVhc2Ug
bm90ZSB0aGUgYnJlYWsgc3RhdGVtZW50IGF0DQo+ID4gdGhlDQo+ID4gZW5kIG9mIHRoZSB3aGls
ZSBsb29wLiAgVGhpcyBwYXRjaCBjaGFuZ2VzIHRoZSBsb29wIHNvIHRoYXQgaWYgaXQncw0KPiA+
IG5vdA0KPiA+IHN1Y2Nlc3NmdWwgdGhlbiAicmVwbHkiIGlzIE5VTEwgYW5kIHdlIGNhbiB0ZXN0
IGZvciB0aGF0IGFmdGVyd2FyZC4NCj4gPiANCj4gPiBGaXhlczogNmI3YzNiODZmMGI2ICgiZHJt
L3Ztd2dmeDogZml4IG1lbW9yeSBsZWFrIHdoZW4gdG9vIG1hbnkNCj4gPiByZXRyaWVzIGhhdmUg
b2NjdXJyZWQiKQ0KPiA+IFNpZ25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRhbi5jYXJwZW50
ZXJAb3JhY2xlLmNvbT4NCj4gPiAtLS0NCj4gPiAgZHJpdmVycy9ncHUvZHJtL3Ztd2dmeC92bXdn
ZnhfbXNnLmMgfCA4ICsrKy0tLS0tDQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlvbnMo
KyksIDUgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2Ry
bS92bXdnZngvdm13Z2Z4X21zZy5jDQo+ID4gYi9kcml2ZXJzL2dwdS9kcm0vdm13Z2Z4L3Ztd2dm
eF9tc2cuYw0KPiA+IGluZGV4IDU5ZTlkMDVhYjkyOC4uMGFmMDQ4ZDFhODE1IDEwMDY0NA0KPiA+
IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS92bXdnZngvdm13Z2Z4X21zZy5jDQo+ID4gKysrIGIvZHJp
dmVycy9ncHUvZHJtL3Ztd2dmeC92bXdnZnhfbXNnLmMNCj4gPiBAQCAtMzUzLDcgKzM1Myw3IEBA
IHN0YXRpYyBpbnQgdm13X3JlY3ZfbXNnKHN0cnVjdCBycGNfY2hhbm5lbA0KPiA+ICpjaGFubmVs
LCB2b2lkICoqbXNnLA0KPiA+ICAJCQkJICAgICAhIShISUdIX1dPUkQoZWN4KSAmDQo+ID4gTUVT
U0FHRV9TVEFUVVNfSEIpKTsNCj4gPiAgCQlpZiAoKEhJR0hfV09SRChlYngpICYgTUVTU0FHRV9T
VEFUVVNfU1VDQ0VTUykgPT0gMCkgew0KPiA+ICAJCQlrZnJlZShyZXBseSk7DQo+ID4gLQ0KPiA+
ICsJCQlyZXBseSA9IE5VTEw7DQo+ID4gIAkJCWlmICgoSElHSF9XT1JEKGVieCkgJiBNRVNTQUdF
X1NUQVRVU19DUFQpICE9IDApDQo+ID4gew0KPiA+ICAJCQkJLyogQSBjaGVja3BvaW50IG9jY3Vy
cmVkLiBSZXRyeS4gKi8NCj4gPiAgCQkJCWNvbnRpbnVlOw0KPiA+IEBAIC0zNzcsNyArMzc3LDcg
QEAgc3RhdGljIGludCB2bXdfcmVjdl9tc2coc3RydWN0IHJwY19jaGFubmVsDQo+ID4gKmNoYW5u
ZWwsIHZvaWQgKiptc2csDQo+ID4gIA0KPiA+ICAJCWlmICgoSElHSF9XT1JEKGVjeCkgJiBNRVNT
QUdFX1NUQVRVU19TVUNDRVNTKSA9PSAwKSB7DQo+ID4gIAkJCWtmcmVlKHJlcGx5KTsNCj4gPiAt
DQo+ID4gKwkJCXJlcGx5ID0gTlVMTDsNCj4gPiAgCQkJaWYgKChISUdIX1dPUkQoZWN4KSAmIE1F
U1NBR0VfU1RBVFVTX0NQVCkgIT0gMCkNCj4gPiB7DQo+ID4gIAkJCQkvKiBBIGNoZWNrcG9pbnQg
b2NjdXJyZWQuIFJldHJ5LiAqLw0KPiA+ICAJCQkJY29udGludWU7DQo+ID4gQEAgLTM4OSwxMCAr
Mzg5LDggQEAgc3RhdGljIGludCB2bXdfcmVjdl9tc2coc3RydWN0IHJwY19jaGFubmVsDQo+ID4g
KmNoYW5uZWwsIHZvaWQgKiptc2csDQo+ID4gIAkJYnJlYWs7DQo+ID4gIAl9DQo+ID4gIA0KPiA+
IC0JaWYgKHJldHJpZXMgPT0gUkVUUklFUykgew0KPiA+IC0JCWtmcmVlKHJlcGx5KTsNCj4gPiAr
CWlmICghcmVwbHkpDQo+ID4gIAkJcmV0dXJuIC1FSU5WQUw7DQo+ID4gLQl9DQo+ID4gIA0KPiA+
ICAJKm1zZ19sZW4gPSByZXBseV9sZW47DQo+ID4gIAkqbXNnICAgICA9IHJlcGx5Ow0KPiA+IA0K
PiANCj4gRGFuLCBUaGFua3MgZm9yIGZpeGluZyB1cCBteSBtaXN0YWtlLg0KDQpUaGFua3MsIERh
bi4gU29ycnkgZm9yIHRoZSBsYXRlIHJlcGx5LiANClJldmlld2VkLWJ5OiBUaG9tYXMgSGVsbHN0
csO2bSA8dGhlbGxzdHJvbUB2bXdhcmUuY29tPg0KV2lsbCBwdXNoIHRoaXMgdG8gZml4ZXMuDQoN
Ci9UaG9tYXMNCg0K

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
@ 2019-09-05 11:54     ` Thomas Hellstrom
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Hellstrom @ 2019-09-05 11:54 UTC (permalink / raw)
  To: colin.king, dan.carpenter, Linux-graphics-maintainer
  Cc: airlied, kernel-janitors, dri-devel, linux-kernel

On Thu, 2019-08-15 at 09:38 +0100, Colin Ian King wrote:
> On 15/08/2019 09:30, Dan Carpenter wrote:
> > We recently added a kfree() after the end of the loop:
> > 
> > 	if (retries == RETRIES) {
> > 		kfree(reply);
> > 		return -EINVAL;
> > 	}
> > 
> > There are two problems.  First the test is wrong and because
> > retries
> > equals RETRIES if we succeed on the last iteration through the
> > loop.
> > Second if we fail on the last iteration through the loop then the
> > kfree
> > is a double free.
> > 
> > When you're reading this code, please note the break statement at
> > the
> > end of the while loop.  This patch changes the loop so that if it's
> > not
> > successful then "reply" is NULL and we can test for that afterward.
> > 
> > Fixes: 6b7c3b86f0b6 ("drm/vmwgfx: fix memory leak when too many
> > retries have occurred")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > index 59e9d05ab928..0af048d1a815 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> > @@ -353,7 +353,7 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  				     !!(HIGH_WORD(ecx) &
> > MESSAGE_STATUS_HB));
> >  		if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
> >  			kfree(reply);
> > -
> > +			reply = NULL;
> >  			if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0)
> > {
> >  				/* A checkpoint occurred. Retry. */
> >  				continue;
> > @@ -377,7 +377,7 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  
> >  		if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
> >  			kfree(reply);
> > -
> > +			reply = NULL;
> >  			if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0)
> > {
> >  				/* A checkpoint occurred. Retry. */
> >  				continue;
> > @@ -389,10 +389,8 @@ static int vmw_recv_msg(struct rpc_channel
> > *channel, void **msg,
> >  		break;
> >  	}
> >  
> > -	if (retries == RETRIES) {
> > -		kfree(reply);
> > +	if (!reply)
> >  		return -EINVAL;
> > -	}
> >  
> >  	*msg_len = reply_len;
> >  	*msg     = reply;
> > 
> 
> Dan, Thanks for fixing up my mistake.

Thanks, Dan. Sorry for the late reply. 
Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Will push this to fixes.

/Thomas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-05 11:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-15  8:30 [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg() Dan Carpenter
2019-08-15  8:30 ` Dan Carpenter
2019-08-15  8:38 ` Colin Ian King
2019-08-15  8:38   ` Colin Ian King
2019-09-05 11:54   ` Thomas Hellstrom
2019-09-05 11:54     ` Thomas Hellstrom
2019-09-05 11:54     ` Thomas Hellstrom

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.