* [patch] phonet: some signedness bugs
@ 2011-01-07 20:37 Dan Carpenter
2011-01-10 0:45 ` David Miller
2011-01-10 7:58 ` Rémi Denis-Courmont
0 siblings, 2 replies; 11+ messages in thread
From: Dan Carpenter @ 2011-01-07 20:37 UTC (permalink / raw)
To: Remi Denis-Courmont
Cc: David S. Miller, netdev, kernel-janitors, dan.j.rosenberg
Dan Rosenberg pointed out that there were some signed comparison bugs
in the phonet protocol.
http://marc.info/?l=full-disclosure&m=129424528425330&w=2
If you have already have CAP_SYS_ADMIN then you could use the bugs to
get root, or someone could cause an oops by mistake.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/include/net/phonet/phonet.h b/include/net/phonet/phonet.h
index d5df797..5395e09 100644
--- a/include/net/phonet/phonet.h
+++ b/include/net/phonet/phonet.h
@@ -107,8 +107,8 @@ struct phonet_protocol {
int sock_type;
};
-int phonet_proto_register(int protocol, struct phonet_protocol *pp);
-void phonet_proto_unregister(int protocol, struct phonet_protocol *pp);
+int phonet_proto_register(unsigned int protocol, struct phonet_protocol *pp);
+void phonet_proto_unregister(unsigned int protocol, struct phonet_protocol *pp);
int phonet_sysctl_init(void);
void phonet_sysctl_exit(void);
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index fd95beb..c627d2e 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -37,7 +37,7 @@
/* Transport protocol registration */
static struct phonet_protocol *proto_tab[PHONET_NPROTO] __read_mostly;
-static struct phonet_protocol *phonet_proto_get(int protocol)
+static struct phonet_protocol *phonet_proto_get(unsigned int protocol)
{
struct phonet_protocol *pp;
@@ -60,8 +60,8 @@ static inline void phonet_proto_put(struct phonet_protocol *pp)
/* protocol family functions */
-static int pn_socket_create(struct net *net, struct socket *sock, int protocol,
- int kern)
+static int pn_socket_create(struct net *net, struct socket *sock,
+ unsigned int protocol, int kern)
{
struct sock *sk;
struct pn_sock *pn;
@@ -458,7 +458,7 @@ static struct packet_type phonet_packet_type __read_mostly = {
static DEFINE_MUTEX(proto_tab_lock);
-int __init_or_module phonet_proto_register(int protocol,
+int __init_or_module phonet_proto_register(unsigned int protocol,
struct phonet_protocol *pp)
{
int err = 0;
@@ -481,7 +481,7 @@ int __init_or_module phonet_proto_register(int protocol,
}
EXPORT_SYMBOL(phonet_proto_register);
-void phonet_proto_unregister(int protocol, struct phonet_protocol *pp)
+void phonet_proto_unregister(unsigned int protocol, struct phonet_protocol *pp)
{
mutex_lock(&proto_tab_lock);
BUG_ON(proto_tab[protocol] != pp);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] phonet: some signedness bugs
2011-01-07 20:37 [patch] phonet: some signedness bugs Dan Carpenter
@ 2011-01-10 0:45 ` David Miller
2011-01-10 2:13 ` David Miller
2011-01-10 7:58 ` Rémi Denis-Courmont
1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2011-01-10 0:45 UTC (permalink / raw)
To: error27; +Cc: remi.denis-courmont, netdev, kernel-janitors, dan.j.rosenberg
From: Dan Carpenter <error27@gmail.com>
Date: Fri, 7 Jan 2011 23:37:55 +0300
> Dan Rosenberg pointed out that there were some signed comparison bugs
> in the phonet protocol.
>
> http://marc.info/?l=full-disclosure&m=129424528425330&w=2
>
> If you have already have CAP_SYS_ADMIN then you could use the bugs to
> get root, or someone could cause an oops by mistake.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied and queued up for -stable, thanks Dan.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] phonet: some signedness bugs
2011-01-10 0:45 ` David Miller
@ 2011-01-10 2:13 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2011-01-10 2:13 UTC (permalink / raw)
To: error27; +Cc: remi.denis-courmont, netdev, kernel-janitors, dan.j.rosenberg
From: David Miller <davem@davemloft.net>
Date: Sun, 09 Jan 2011 16:45:48 -0800 (PST)
> From: Dan Carpenter <error27@gmail.com>
> Date: Fri, 7 Jan 2011 23:37:55 +0300
>
>> Dan Rosenberg pointed out that there were some signed comparison bugs
>> in the phonet protocol.
>>
>> http://marc.info/?l=full-disclosure&m=129424528425330&w=2
>>
>> If you have already have CAP_SYS_ADMIN then you could use the bugs to
>> get root, or someone could cause an oops by mistake.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> Applied and queued up for -stable, thanks Dan.
Actually I'm reverting this.
You can't change the prototype of pn_socket_create() because if you do
then it doesn't match up with the prototype required by
net_proto_family->create().
You didn't see this warning in your build?
net/phonet/af_phonet.c:124:2: warning: initialization from incompatible pointer type
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] phonet: some signedness bugs
2011-01-07 20:37 [patch] phonet: some signedness bugs Dan Carpenter
2011-01-10 0:45 ` David Miller
@ 2011-01-10 7:58 ` Rémi Denis-Courmont
2011-01-10 14:01 ` Dan Carpenter
2011-01-10 14:06 ` [patch v2] " Dan Carpenter
1 sibling, 2 replies; 11+ messages in thread
From: Rémi Denis-Courmont @ 2011-01-10 7:58 UTC (permalink / raw)
To: ext Dan Carpenter
Cc: David S. Miller, netdev, kernel-janitors, dan.j.rosenberg
On Friday 07 January 2011 22:37:55 ext Dan Carpenter, you wrote:
> Dan Rosenberg pointed out that there were some signed comparison bugs
> in the phonet protocol.
There are two ways to solve this: change *only* the proto_get function to use
an unsigned parameter, or cast the protocol to unsigned in the comparison.
As David pointed out, your patch breaks the socket() callback prototype.
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] phonet: some signedness bugs
2011-01-10 7:58 ` Rémi Denis-Courmont
@ 2011-01-10 14:01 ` Dan Carpenter
2011-01-10 14:08 ` Rémi Denis-Courmont
2011-01-10 14:06 ` [patch v2] " Dan Carpenter
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2011-01-10 14:01 UTC (permalink / raw)
To: Rémi Denis-Courmont
Cc: David S. Miller, netdev, kernel-janitors, dan.j.rosenberg
On Mon, Jan 10, 2011 at 09:58:32AM +0200, Rémi Denis-Courmont wrote:
> On Friday 07 January 2011 22:37:55 ext Dan Carpenter, you wrote:
> > Dan Rosenberg pointed out that there were some signed comparison bugs
> > in the phonet protocol.
>
> There are two ways to solve this: change *only* the proto_get function to use
> an unsigned parameter, or cast the protocol to unsigned in the comparison.
>
> As David pointed out, your patch breaks the socket() callback prototype.
>
Yes. I really appologize for that. I'll send version 2 with that create()
change dropped. It's not needed.
I would like to keep the change to phonet_proto_register() because the
check in there isn't right and it's similar to phonet_proto_get(). We
may as well keep phonet_proto_unregister() as well so it's symmetric.
Let me know if you disagree and I'll redo it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* [patch v2] phonet: some signedness bugs
2011-01-10 7:58 ` Rémi Denis-Courmont
2011-01-10 14:01 ` Dan Carpenter
@ 2011-01-10 14:06 ` Dan Carpenter
2011-01-10 14:12 ` Rémi Denis-Courmont
2011-01-11 0:06 ` David Miller
1 sibling, 2 replies; 11+ messages in thread
From: Dan Carpenter @ 2011-01-10 14:06 UTC (permalink / raw)
To: Rémi Denis-Courmont
Cc: David S. Miller, netdev, kernel-janitors, dan.j.rosenberg
Dan Rosenberg pointed out that there were some signed comparison bugs
in the phonet protocol.
http://marc.info/?l=full-disclosure&m=129424528425330&w=2
The problem is that we check for array overflows but "protocol" is
signed and we don't check for array underflows. If you have already
have CAP_SYS_ADMIN then you could use the bugs to get root, or someone
could cause an oops by mistake.
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: in v1 I changed pn_socket_create() but that change caused a
compiler warning. That part of the patch wasn't needed so I've just
dropped.
diff --git a/include/net/phonet/phonet.h b/include/net/phonet/phonet.h
index d5df797..5395e09 100644
--- a/include/net/phonet/phonet.h
+++ b/include/net/phonet/phonet.h
@@ -107,8 +107,8 @@ struct phonet_protocol {
int sock_type;
};
-int phonet_proto_register(int protocol, struct phonet_protocol *pp);
-void phonet_proto_unregister(int protocol, struct phonet_protocol *pp);
+int phonet_proto_register(unsigned int protocol, struct phonet_protocol *pp);
+void phonet_proto_unregister(unsigned int protocol, struct phonet_protocol *pp);
int phonet_sysctl_init(void);
void phonet_sysctl_exit(void);
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index fd95beb..1072b2c 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -37,7 +37,7 @@
/* Transport protocol registration */
static struct phonet_protocol *proto_tab[PHONET_NPROTO] __read_mostly;
-static struct phonet_protocol *phonet_proto_get(int protocol)
+static struct phonet_protocol *phonet_proto_get(unsigned int protocol)
{
struct phonet_protocol *pp;
@@ -458,7 +458,7 @@ static struct packet_type phonet_packet_type __read_mostly = {
static DEFINE_MUTEX(proto_tab_lock);
-int __init_or_module phonet_proto_register(int protocol,
+int __init_or_module phonet_proto_register(unsigned int protocol,
struct phonet_protocol *pp)
{
int err = 0;
@@ -481,7 +481,7 @@ int __init_or_module phonet_proto_register(int protocol,
}
EXPORT_SYMBOL(phonet_proto_register);
-void phonet_proto_unregister(int protocol, struct phonet_protocol *pp)
+void phonet_proto_unregister(unsigned int protocol, struct phonet_protocol *pp)
{
mutex_lock(&proto_tab_lock);
BUG_ON(proto_tab[protocol] != pp);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] phonet: some signedness bugs
2011-01-10 14:01 ` Dan Carpenter
@ 2011-01-10 14:08 ` Rémi Denis-Courmont
0 siblings, 0 replies; 11+ messages in thread
From: Rémi Denis-Courmont @ 2011-01-10 14:08 UTC (permalink / raw)
To: ext Dan Carpenter
Cc: David S. Miller, netdev, kernel-janitors, dan.j.rosenberg
On Monday 10 January 2011 16:01:41 ext Dan Carpenter, you wrote:
> I would like to keep the change to phonet_proto_register() because the
> check in there isn't right and it's similar to phonet_proto_get(). We
> may as well keep phonet_proto_unregister() as well so it's symmetric.
> Let me know if you disagree and I'll redo it.
Sounds sensible to me.
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch v2] phonet: some signedness bugs
2011-01-10 14:06 ` [patch v2] " Dan Carpenter
@ 2011-01-10 14:12 ` Rémi Denis-Courmont
2011-01-11 0:06 ` David Miller
1 sibling, 0 replies; 11+ messages in thread
From: Rémi Denis-Courmont @ 2011-01-10 14:12 UTC (permalink / raw)
To: ext Dan Carpenter; +Cc: netdev, kernel-janitors, dan.j.rosenberg
On Monday 10 January 2011 16:06:58 ext Dan Carpenter, you wrote:
> Dan Rosenberg pointed out that there were some signed comparison bugs
> in the phonet protocol.
>
> http://marc.info/?l=full-disclosure&m=129424528425330&w=2
>
> The problem is that we check for array overflows but "protocol" is
> signed and we don't check for array underflows. If you have already
> have CAP_SYS_ADMIN then you could use the bugs to get root, or someone
> could cause an oops by mistake.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch v2] phonet: some signedness bugs
2011-01-10 14:06 ` [patch v2] " Dan Carpenter
2011-01-10 14:12 ` Rémi Denis-Courmont
@ 2011-01-11 0:06 ` David Miller
2011-01-13 12:32 ` Rémi Denis-Courmont
1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2011-01-11 0:06 UTC (permalink / raw)
To: error27; +Cc: remi.denis-courmont, netdev, kernel-janitors, dan.j.rosenberg
From: Dan Carpenter <error27@gmail.com>
Date: Mon, 10 Jan 2011 17:06:58 +0300
> Dan Rosenberg pointed out that there were some signed comparison bugs
> in the phonet protocol.
>
> http://marc.info/?l=full-disclosure&m=129424528425330&w=2
>
> The problem is that we check for array overflows but "protocol" is
> signed and we don't check for array underflows. If you have already
> have CAP_SYS_ADMIN then you could use the bugs to get root, or someone
> could cause an oops by mistake.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch v2] phonet: some signedness bugs
2011-01-11 0:06 ` David Miller
@ 2011-01-13 12:32 ` Rémi Denis-Courmont
2011-01-13 20:30 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Rémi Denis-Courmont @ 2011-01-13 12:32 UTC (permalink / raw)
To: error27, dan.j.rosenberg; +Cc: netdev, kernel-janitors
On Tuesday 11 January 2011 02:06:20 ext David Miller, you wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Mon, 10 Jan 2011 17:06:58 +0300
>
> > Dan Rosenberg pointed out that there were some signed comparison bugs
> > in the phonet protocol.
> >
> > http://marc.info/?l=full-disclosure&m=129424528425330&w=2
> >
> > The problem is that we check for array overflows but "protocol" is
> > signed and we don't check for array underflows. If you have already
> > have CAP_SYS_ADMIN then you could use the bugs to get root, or someone
> > could cause an oops by mistake.
> >
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> Applied.
Shouldn't this be sent to stable trees?
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch v2] phonet: some signedness bugs
2011-01-13 12:32 ` Rémi Denis-Courmont
@ 2011-01-13 20:30 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2011-01-13 20:30 UTC (permalink / raw)
To: remi.denis-courmont; +Cc: error27, dan.j.rosenberg, netdev, kernel-janitors
From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com>
Date: Thu, 13 Jan 2011 14:32:57 +0200
> On Tuesday 11 January 2011 02:06:20 ext David Miller, you wrote:
>> From: Dan Carpenter <error27@gmail.com>
>> Date: Mon, 10 Jan 2011 17:06:58 +0300
>>
>> > Dan Rosenberg pointed out that there were some signed comparison bugs
>> > in the phonet protocol.
>> >
>> > http://marc.info/?l=full-disclosure&m=129424528425330&w=2
>> >
>> > The problem is that we check for array overflows but "protocol" is
>> > signed and we don't check for array underflows. If you have already
>> > have CAP_SYS_ADMIN then you could use the bugs to get root, or someone
>> > could cause an oops by mistake.
>> >
>> > Signed-off-by: Dan Carpenter <error27@gmail.com>
>>
>> Applied.
>
> Shouldn't this be sent to stable trees?
It will be.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-01-13 20:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-07 20:37 [patch] phonet: some signedness bugs Dan Carpenter
2011-01-10 0:45 ` David Miller
2011-01-10 2:13 ` David Miller
2011-01-10 7:58 ` Rémi Denis-Courmont
2011-01-10 14:01 ` Dan Carpenter
2011-01-10 14:08 ` Rémi Denis-Courmont
2011-01-10 14:06 ` [patch v2] " Dan Carpenter
2011-01-10 14:12 ` Rémi Denis-Courmont
2011-01-11 0:06 ` David Miller
2011-01-13 12:32 ` Rémi Denis-Courmont
2011-01-13 20:30 ` David Miller
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).