[RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[Cyrill Gorcunov]

Hi,

while were sneaking thru sockets code I've got the idea that we may
check for __kernel_sockaddr_storage overflow at build time. At moment
this structure is big enough and I hardly believe it could be overflowed
ever (hmm?).

Anyway just an idea which could be stupid perhaps but I decided to
put it out. An idea is that before copy protocol specific data in
socket->ops->getname implementation the driver code may put

build_sockaddr_check(sizeof(some_struct));

and be sure it doesn't overflow the hosting unit.

Feel free to just ignore this RFC, was just an idea to share.

	-- Cyrill
---
net,socket: introduce build_sockaddr_check helper to catch overflow at build time

proto_ops->getname implies copying protocol specific data
into storage unit (particulary to __kernel_sockaddr_storage).
So when one implements new protocol he either may keep this
in mind (or may not).

Lets introduce build_sockaddr_check helper which check if
storage unit is not overfowed. Note that the check is build
time and introduce no slowdown at execution time.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 include/linux/socket.h |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.git/include/linux/socket.h
=====================================================================
--- linux-2.6.git.orig/include/linux/socket.h
+++ linux-2.6.git/include/linux/socket.h
@@ -24,6 +24,9 @@ struct __kernel_sockaddr_storage {
 #include <linux/types.h>		/* pid_t			*/
 #include <linux/compiler.h>		/* __user			*/
 
+#define build_sockaddr_check(size)	\
+	BUILD_BUG_ON(((size) > sizeof(struct __kernel_sockaddr_storage)))
+
 #ifdef __KERNEL__
 # ifdef CONFIG_PROC_FS
 struct seq_file;

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[David Miller]

From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Wed, 21 Oct 2009 21:07:32 +0400

> net,socket: introduce build_sockaddr_check helper to catch overflow at build time
> 
> proto_ops->getname implies copying protocol specific data
> into storage unit (particulary to __kernel_sockaddr_storage).
> So when one implements new protocol he either may keep this
> in mind (or may not).
> 
> Lets introduce build_sockaddr_check helper which check if
> storage unit is not overfowed. Note that the check is build
> time and introduce no slowdown at execution time.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Nice idea, and I wonder if we can automate it even further.
Perhaps some tag that gets put on the socket address type
definition or similar?

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[Cyrill Gorcunov]

[David Miller - Thu, Oct 22, 2009 at 04:49:14AM -0700]
| From: Cyrill Gorcunov <gorcunov@gmail.com>
| Date: Wed, 21 Oct 2009 21:07:32 +0400
| 
| > net,socket: introduce build_sockaddr_check helper to catch overflow at build time
| > 
| > proto_ops->getname implies copying protocol specific data
| > into storage unit (particulary to __kernel_sockaddr_storage).
| > So when one implements new protocol he either may keep this
| > in mind (or may not).
| > 
| > Lets introduce build_sockaddr_check helper which check if
| > storage unit is not overfowed. Note that the check is build
| > time and introduce no slowdown at execution time.
| > 
| > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| 
| Nice idea, and I wonder if we can automate it even further.
| Perhaps some tag that gets put on the socket address type
| definition or similar?
| 

Thanks for review David! Not sure if I understand you right.
Initially I was trying to bring as minimum changes as possible.
Also I was shuffle in mind the following possibilities:

1) Since at least one .getname handler use memcpy, we could
   introduce some helper which check size (at build time) and
   then do memcpy (not optimal perhaps).


2) All handlers set *len to some size explicitly so we may
   introduce set_sockaddr_size() helper like

#define set_sockaddr_size(ptr, size)		\
	do {					\
		build_sockaddr_check(size);	\
		*ptr = size;			\
	} while (0)

Or you meant something completely different?

	-- Cyrill

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[Cyrill Gorcunov]

[Cyrill Gorcunov - Thu, Oct 22, 2009 at 05:55:57PM +0400]
...
| 
| 2) All handlers set *len to some size explicitly so we may
|    introduce set_sockaddr_size() helper like
| 
| #define set_sockaddr_size(ptr, size)		\
| 	do {					\
| 		build_sockaddr_check(size);	\
| 		*ptr = size;			\
| 	} while (0)
| 
| Or you meant something completely different?
| 
| 	-- Cyrill

Or say it could be something like that

#define __sockaddr(type, src)	\
	({ build_sockaddr_check(sizeof(type)); (type *) src; })

and say in function af_inet.c:inet_getname instead of

	struct sockaddr_in *sin	= (struct sockaddr_in *)uaddr;

we may write like

	struct sockaddr_in *sin	= __sockaddr(struct sockaddr_in, uaddr);

which would check the size.

	-- Cyrill

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[David Miller]

From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Sat, 24 Oct 2009 01:43:06 +0400

> Or say it could be something like that
> 
> #define __sockaddr(type, src)	\
> 	({ build_sockaddr_check(sizeof(type)); (type *) src; })
> 
> and say in function af_inet.c:inet_getname instead of
> 
> 	struct sockaddr_in *sin	= (struct sockaddr_in *)uaddr;
> 
> we may write like
> 
> 	struct sockaddr_in *sin	= __sockaddr(struct sockaddr_in, uaddr);
> 
> which would check the size.

Or even a "DECLARE_SOCKADDR(type, src, dest)" which encapsulates the
entire declaration statement.

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[Cyrill Gorcunov]

[David Miller - Sat, Oct 24, 2009 at 06:12:09AM -0700]
| From: Cyrill Gorcunov <gorcunov@gmail.com>
| Date: Sat, 24 Oct 2009 01:43:06 +0400
| 
| > Or say it could be something like that
| > 
| > #define __sockaddr(type, src)	\
| > 	({ build_sockaddr_check(sizeof(type)); (type *) src; })
| > 
| > and say in function af_inet.c:inet_getname instead of
| > 
| > 	struct sockaddr_in *sin	= (struct sockaddr_in *)uaddr;
| > 
| > we may write like
| > 
| > 	struct sockaddr_in *sin	= __sockaddr(struct sockaddr_in, uaddr);
| > 
| > which would check the size.
| 
| Or even a "DECLARE_SOCKADDR(type, src, dest)" which encapsulates the
| entire declaration statement.
| 

Something like this I suppose?

	-- Cyrill
---
net,socket: introduce DECLARE_SOCKADDR helper to catch overflow at build time

proto_ops->getname implies copying protocol specific data
into storage unit (particulary to __kernel_sockaddr_storage).
So when we implement new protocol support we should keep such
a detail in mind (which is easy to forget about).

Lets introduce DECLARE_SOCKADDR helper which check if
storage unit is not overfowed at build time.

Eventually inet_getname is switched to use DECLARE_SOCKADDR
(to show example of usage).

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 include/linux/net.h    |    3 +++
 include/linux/socket.h |    3 +++
 net/ipv4/af_inet.c     |    2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6.git/include/linux/net.h
=====================================================================
--- linux-2.6.git.orig/include/linux/net.h
+++ linux-2.6.git/include/linux/net.h
@@ -198,6 +198,9 @@ struct proto_ops {
 				       struct pipe_inode_info *pipe, size_t len, unsigned int flags);
 };
 
+#define DECLARE_SOCKADDR(type, dst, src)	\
+	type dst = ({ __sockaddr_check_size(sizeof(*dst)); (type) src; })
+
 struct net_proto_family {
 	int		family;
 	int		(*create)(struct net *net, struct socket *sock, int protocol);
Index: linux-2.6.git/include/linux/socket.h
=====================================================================
--- linux-2.6.git.orig/include/linux/socket.h
+++ linux-2.6.git/include/linux/socket.h
@@ -24,6 +24,9 @@ struct __kernel_sockaddr_storage {
 #include <linux/types.h>		/* pid_t			*/
 #include <linux/compiler.h>		/* __user			*/
 
+#define __sockaddr_check_size(size)	\
+	BUILD_BUG_ON(((size) > sizeof(struct __kernel_sockaddr_storage)))
+
 #ifdef __KERNEL__
 # ifdef CONFIG_PROC_FS
 struct seq_file;
Index: linux-2.6.git/net/ipv4/af_inet.c
=====================================================================
--- linux-2.6.git.orig/net/ipv4/af_inet.c
+++ linux-2.6.git/net/ipv4/af_inet.c
@@ -685,7 +685,7 @@ int inet_getname(struct socket *sock, st
 {
 	struct sock *sk		= sock->sk;
 	struct inet_sock *inet	= inet_sk(sk);
-	struct sockaddr_in *sin	= (struct sockaddr_in *)uaddr;
+	DECLARE_SOCKADDR(struct sockaddr_in *, sin, uaddr);
 
 	sin->sin_family = AF_INET;
 	if (peer) {

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[David Miller]

From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Sat, 24 Oct 2009 20:32:26 +0400

> net,socket: introduce DECLARE_SOCKADDR helper to catch overflow at build time
> 
> proto_ops->getname implies copying protocol specific data
> into storage unit (particulary to __kernel_sockaddr_storage).
> So when we implement new protocol support we should keep such
> a detail in mind (which is easy to forget about).
> 
> Lets introduce DECLARE_SOCKADDR helper which check if
> storage unit is not overfowed at build time.
> 
> Eventually inet_getname is switched to use DECLARE_SOCKADDR
> (to show example of usage).
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

I like this, applied to net-next-2.6, thanks!

Re: [RFC] net,socket: introduce build_sockaddr_check helper to catch overflow at build time

[Cyrill Gorcunov]

[David Miller - Thu, Oct 29, 2009 at 03:00:19AM -0700]
...
| > Eventually inet_getname is switched to use DECLARE_SOCKADDR
| > (to show example of usage).
| > 
| > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| 
| I like this, applied to net-next-2.6, thanks!
| 

Thanks David. I'll handle other protocols in a couple
of days.

	-- Cyrill