* [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
0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* Re: [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg()
2019-08-15 8:30 [PATCH] drm/vmwgfx: Fix double free in vmw_recv_msg() Dan Carpenter
@ 2019-08-15 8:38 ` Colin Ian King
2019-09-05 11:54 ` Thomas Hellstrom
0 siblings, 1 reply; 3+ 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] 3+ 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
0 siblings, 0 replies; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2019-09-05 11:55 UTC | newest]
Thread overview: 3+ 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:38 ` Colin Ian King
2019-09-05 11:54 ` Thomas Hellstrom
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).