* [patch] target: use after free in error handling
@ 2011-08-09 12:30 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2011-08-09 12:30 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
iscsit_release_cmd() frees the memory that "se_cmd" was pointing to
so this is a use after free bug. Also "se_cmd" is non-null here so I
removed the unneeded null check.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index a1acb01..bea5c29 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -297,9 +297,8 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
return cmd;
out:
+ transport_free_se_cmd(se_cmd);
iscsit_release_cmd(cmd);
- if (se_cmd)
- transport_free_se_cmd(se_cmd);
return NULL;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch] target: use after free in error handling
@ 2011-08-09 12:30 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2011-08-09 12:30 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
iscsit_release_cmd() frees the memory that "se_cmd" was pointing to
so this is a use after free bug. Also "se_cmd" is non-null here so I
removed the unneeded null check.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index a1acb01..bea5c29 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -297,9 +297,8 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
return cmd;
out:
+ transport_free_se_cmd(se_cmd);
iscsit_release_cmd(cmd);
- if (se_cmd)
- transport_free_se_cmd(se_cmd);
return NULL;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] target: use after free in error handling
2011-08-09 12:30 ` Dan Carpenter
@ 2011-08-14 5:36 ` Nicholas A. Bellinger
-1 siblings, 0 replies; 6+ messages in thread
From: Nicholas A. Bellinger @ 2011-08-14 5:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
On Tue, 2011-08-09 at 15:30 +0300, Dan Carpenter wrote:
> iscsit_release_cmd() frees the memory that "se_cmd" was pointing to
> so this is a use after free bug. Also "se_cmd" is non-null here so I
> removed the unneeded null check.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
Hi Dan,
Apologies for the delay here..
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index a1acb01..bea5c29 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -297,9 +297,8 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
>
> return cmd;
> out:
> + transport_free_se_cmd(se_cmd);
> iscsit_release_cmd(cmd);
> - if (se_cmd)
> - transport_free_se_cmd(se_cmd);
> return NULL;
> }
>
So the transport_free_se_cmd() call is not necessary as this point, and
can be safely dropped with the single call to iscsit_release_cmd().
Committing the following patch into lio-core-2.6.git with one additional
missing case in iscsit_allocate_se_cmd_for_tmr().
Thank you,
--nab
commit 81d779b23ef4e33a40f558843af5122cccb2f913
Author: Dan Carpenter <error27@gmail.com>
Date: Sat Aug 13 22:35:00 2011 -0700
iscsi-target: Fix iscsit_allocate_se_cmd_for_tmr failure path bugs
This patch fixes two bugs in allocation failure handling in
iscsit_allocate_se_cmd_for_tmr():
This first reported by DanC is a free-after call to transport_free_se_cmd(), this
patch drops the transport_free_se_cmd() call all together, as iscsit_release_cmd()
will release existing allocations as expected.
The second is a bug where iscsi_cmd_t was being leaked on a cmd->tmr_req allocation
failure, so make this jump to iscsit_release_cmd() as well.
Signed-off-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index a9e7add..20242b5 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -243,7 +243,7 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
if (!cmd->tmr_req) {
pr_err("Unable to allocate memory for"
" Task Management command!\n");
- return NULL;
+ goto out;
}
/*
* TASK_REASSIGN for ERL=2 / connection stays inside of
@@ -298,8 +298,6 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
return cmd;
out:
iscsit_release_cmd(cmd);
- if (se_cmd)
- transport_free_se_cmd(se_cmd);
return NULL;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] target: use after free in error handling
@ 2011-08-14 5:36 ` Nicholas A. Bellinger
0 siblings, 0 replies; 6+ messages in thread
From: Nicholas A. Bellinger @ 2011-08-14 5:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
On Tue, 2011-08-09 at 15:30 +0300, Dan Carpenter wrote:
> iscsit_release_cmd() frees the memory that "se_cmd" was pointing to
> so this is a use after free bug. Also "se_cmd" is non-null here so I
> removed the unneeded null check.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
Hi Dan,
Apologies for the delay here..
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index a1acb01..bea5c29 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -297,9 +297,8 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
>
> return cmd;
> out:
> + transport_free_se_cmd(se_cmd);
> iscsit_release_cmd(cmd);
> - if (se_cmd)
> - transport_free_se_cmd(se_cmd);
> return NULL;
> }
>
So the transport_free_se_cmd() call is not necessary as this point, and
can be safely dropped with the single call to iscsit_release_cmd().
Committing the following patch into lio-core-2.6.git with one additional
missing case in iscsit_allocate_se_cmd_for_tmr().
Thank you,
--nab
commit 81d779b23ef4e33a40f558843af5122cccb2f913
Author: Dan Carpenter <error27@gmail.com>
Date: Sat Aug 13 22:35:00 2011 -0700
iscsi-target: Fix iscsit_allocate_se_cmd_for_tmr failure path bugs
This patch fixes two bugs in allocation failure handling in
iscsit_allocate_se_cmd_for_tmr():
This first reported by DanC is a free-after call to transport_free_se_cmd(), this
patch drops the transport_free_se_cmd() call all together, as iscsit_release_cmd()
will release existing allocations as expected.
The second is a bug where iscsi_cmd_t was being leaked on a cmd->tmr_req allocation
failure, so make this jump to iscsit_release_cmd() as well.
Signed-off-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index a9e7add..20242b5 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -243,7 +243,7 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
if (!cmd->tmr_req) {
pr_err("Unable to allocate memory for"
" Task Management command!\n");
- return NULL;
+ goto out;
}
/*
* TASK_REASSIGN for ERL=2 / connection stays inside of
@@ -298,8 +298,6 @@ struct iscsi_cmd *iscsit_allocate_se_cmd_for_tmr(
return cmd;
out:
iscsit_release_cmd(cmd);
- if (se_cmd)
- transport_free_se_cmd(se_cmd);
return NULL;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] target: use after free in error handling
2011-08-14 5:36 ` Nicholas A. Bellinger
@ 2011-08-14 19:10 ` Dan Carpenter
-1 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2011-08-14 19:10 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
Ah yes... I should have caught the missing goto out. I hardly feel
like I deserve the Author tag for this one but I'm always happy to
take credit for other people's hard work. Moving up the corporate
ladder and all that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] target: use after free in error handling
@ 2011-08-14 19:10 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2011-08-14 19:10 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Andy Grover, Roland Dreier, Christoph Hellwig,
open list:TARGET SUBSYSTEM, open list:TARGET SUBSYSTEM,
kernel-janitors
Ah yes... I should have caught the missing goto out. I hardly feel
like I deserve the Author tag for this one but I'm always happy to
take credit for other people's hard work. Moving up the corporate
ladder and all that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-08-14 19:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-09 12:30 [patch] target: use after free in error handling Dan Carpenter
2011-08-09 12:30 ` Dan Carpenter
2011-08-14 5:36 ` Nicholas A. Bellinger
2011-08-14 5:36 ` Nicholas A. Bellinger
2011-08-14 19:10 ` Dan Carpenter
2011-08-14 19:10 ` Dan Carpenter
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.