Patchwork phonet: some signedness bugs

login
register
mail settings
Submitter Dan Carpenter
Date Jan. 7, 2011, 8:37 p.m.
Message ID <20110107203755.GB1959@bicker>
Download mbox | patch
Permalink /patch/77908/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Dan Carpenter - Jan. 7, 2011, 8:37 p.m.
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>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 10, 2011, 12:45 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 10, 2011, 2:13 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rémi Denis-Courmont - Jan. 10, 2011, 7:58 a.m.
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.
Dan Carpenter - Jan. 10, 2011, 2:01 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rémi Denis-Courmont - Jan. 10, 2011, 2:08 p.m.
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.

Patch

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);