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

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(+)

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

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?
--
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 - 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
--
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

[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
--
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

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.
--
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