* [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
@ 2010-12-25 18:17 Jesper Juhl
2010-12-25 21:12 ` richard -rw- weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2010-12-25 18:17 UTC (permalink / raw)
To: ceph-devel; +Cc: linux-kernel, netdev, Sage Weil, David S. Miller
Hello,
In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
freed by kfree() and subsequently used in a call to dout() - use after
free bug.
Easily fixed by simply moving the kfree() call after the dout() call.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
ceph_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index f3e4a13..890bbbf 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -408,8 +408,8 @@ void ceph_destroy_client(struct ceph_client *client)
ceph_destroy_options(client->options);
- kfree(client);
dout("destroy_client %p done\n", client);
+ kfree(client);
}
EXPORT_SYMBOL(ceph_destroy_client);
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
2010-12-25 18:17 [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client() Jesper Juhl
@ 2010-12-25 21:12 ` richard -rw- weinberger
2010-12-25 21:24 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: richard -rw- weinberger @ 2010-12-25 21:12 UTC (permalink / raw)
To: Jesper Juhl; +Cc: ceph-devel, linux-kernel, netdev, Sage Weil, David S. Miller
On Sat, Dec 25, 2010 at 7:17 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> Hello,
>
> In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
> freed by kfree() and subsequently used in a call to dout() - use after
> free bug.
Not really. %p reads only the address of "client".
kfree() does not alter this address.
> Easily fixed by simply moving the kfree() call after the dout() call.
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
> ceph_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index f3e4a13..890bbbf 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -408,8 +408,8 @@ void ceph_destroy_client(struct ceph_client *client)
>
> ceph_destroy_options(client->options);
>
> - kfree(client);
> dout("destroy_client %p done\n", client);
> + kfree(client);
> }
> EXPORT_SYMBOL(ceph_destroy_client);
>
>
> --
> Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
2010-12-25 21:12 ` richard -rw- weinberger
@ 2010-12-25 21:24 ` Jesper Juhl
2010-12-25 21:40 ` richard -rw- weinberger
2010-12-25 22:46 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Jesper Juhl @ 2010-12-25 21:24 UTC (permalink / raw)
To: richard -rw- weinberger
Cc: ceph-devel, linux-kernel, netdev, Sage Weil, David S. Miller
On Sat, 25 Dec 2010, richard -rw- weinberger wrote:
> On Sat, Dec 25, 2010 at 7:17 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> > Hello,
> >
> > In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
> > freed by kfree() and subsequently used in a call to dout() - use after
> > free bug.
>
> Not really. %p reads only the address of "client".
> kfree() does not alter this address.
>
Ok, I see your point and you are correct. But still, the patch does not
change behaviour and it makes it absolutely clear that there's no
use-after-free bug, so it might still have merit... or?
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
2010-12-25 21:24 ` Jesper Juhl
@ 2010-12-25 21:40 ` richard -rw- weinberger
2010-12-25 21:35 ` Jesper Juhl
2010-12-25 22:46 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: richard -rw- weinberger @ 2010-12-25 21:40 UTC (permalink / raw)
To: Jesper Juhl; +Cc: ceph-devel, linux-kernel, netdev, Sage Weil, David S. Miller
On Sat, Dec 25, 2010 at 10:24 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> On Sat, 25 Dec 2010, richard -rw- weinberger wrote:
>
>> On Sat, Dec 25, 2010 at 7:17 PM, Jesper Juhl <jj@chaosbits.net> wrote:
>> > Hello,
>> >
>> > In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
>> > freed by kfree() and subsequently used in a call to dout() - use after
>> > free bug.
>>
>> Not really. %p reads only the address of "client".
>> kfree() does not alter this address.
>>
>
> Ok, I see your point and you are correct. But still, the patch does not
> change behaviour and it makes it absolutely clear that there's no
> use-after-free bug, so it might still have merit... or?
Your patch does not fix a bug.
I would say it's a style fix.
> --
> Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please.
>
>
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
2010-12-25 21:40 ` richard -rw- weinberger
@ 2010-12-25 21:35 ` Jesper Juhl
0 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2010-12-25 21:35 UTC (permalink / raw)
To: richard -rw- weinberger
Cc: ceph-devel, linux-kernel, netdev, Sage Weil, David S. Miller
On Sat, 25 Dec 2010, richard -rw- weinberger wrote:
> On Sat, Dec 25, 2010 at 10:24 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> > On Sat, 25 Dec 2010, richard -rw- weinberger wrote:
> >
> >> On Sat, Dec 25, 2010 at 7:17 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> >> > Hello,
> >> >
> >> > In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
> >> > freed by kfree() and subsequently used in a call to dout() - use after
> >> > free bug.
> >>
> >> Not really. %p reads only the address of "client".
> >> kfree() does not alter this address.
> >>
> >
> > Ok, I see your point and you are correct. But still, the patch does not
> > change behaviour and it makes it absolutely clear that there's no
> > use-after-free bug, so it might still have merit... or?
>
> Your patch does not fix a bug.
> I would say it's a style fix.
>
At this point in time I'd agree. :-)
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client().
2010-12-25 21:24 ` Jesper Juhl
2010-12-25 21:40 ` richard -rw- weinberger
@ 2010-12-25 22:46 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2010-12-25 22:46 UTC (permalink / raw)
To: Jesper Juhl
Cc: richard -rw- weinberger, ceph-devel, linux-kernel, netdev,
Sage Weil, David S. Miller
On Sat, Dec 25, 2010 at 10:24:57PM +0100, Jesper Juhl wrote:
> On Sat, 25 Dec 2010, richard -rw- weinberger wrote:
>
> > On Sat, Dec 25, 2010 at 7:17 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> > > Hello,
> > >
> > > In net/ceph/ceph_common.c::ceph_destroy_client() the pointer 'client' is
> > > freed by kfree() and subsequently used in a call to dout() - use after
> > > free bug.
> >
> > Not really. %p reads only the address of "client".
> > kfree() does not alter this address.
> >
>
> Ok, I see your point and you are correct. But still, the patch does not
> change behaviour and it makes it absolutely clear that there's no
> use-after-free bug, so it might still have merit... or?
>
I see these with Smatch as well. This type of usage is quite common.
People do it deliberately and I guess they feel it's readable. Don't
change them.
If it were something that a static checker couldn't figure out, then
I'd say change it, but really the static checkers should just be made
smarter. Some day I'm going to make Smatch complain if it's a %s in
the string instead of a %p, but for now I just ignore the false
positives.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-25 22:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-25 18:17 [PATCH] Ceph: Fix a use-after-free bug in ceph_destroy_client() Jesper Juhl
2010-12-25 21:12 ` richard -rw- weinberger
2010-12-25 21:24 ` Jesper Juhl
2010-12-25 21:40 ` richard -rw- weinberger
2010-12-25 21:35 ` Jesper Juhl
2010-12-25 22:46 ` Dan Carpenter
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).