| Message ID | 1288545032-16481-1-git-send-email-segooon@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Vasiliy Kulikov <segooon@gmail.com> Date: Sun, 31 Oct 2010 20:10:32 +0300 > Structure sockaddr_tipc is copied to userland with padding bytes after > "id" field in union field "name" unitialized. It leads to leaking of > contents of kernel stack memory. We have to initialize them to zero. > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> Applied. Patches #1 and #2 were given feedback which I need you to integrate and submit new patches based upon, thanks. -- 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
On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote: > From: Vasiliy Kulikov <segooon@gmail.com> > Date: Sun, 31 Oct 2010 20:10:32 +0300 > > > Structure sockaddr_tipc is copied to userland with padding bytes after > > "id" field in union field "name" unitialized. It leads to leaking of > > contents of kernel stack memory. We have to initialize them to zero. > > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> > > Applied. > > Patches #1 and #2 were given feedback which I need you to integrate > and submit new patches based upon, thanks. About #2: I still think that this: if (dev) strncpy(uaddr->sa_data, dev->name, 14); else memset(uaddr->sa_data, 0, 14); is better than this: memset(uaddr->sa_data, 0, 14); dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex); if (dev) strlcpy(uaddr->sa_data, dev->name, 15); Doesn't it? Explicitly filling with zero on the same "if" level is slightly easier to read and understand.
Am 09.11.2010 21:33, schrieb Vasiliy Kulikov: > On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote: >> From: Vasiliy Kulikov <segooon@gmail.com> >> Date: Sun, 31 Oct 2010 20:10:32 +0300 >> >>> Structure sockaddr_tipc is copied to userland with padding bytes after >>> "id" field in union field "name" unitialized. It leads to leaking of >>> contents of kernel stack memory. We have to initialize them to zero. >>> >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> >> >> Applied. >> >> Patches #1 and #2 were given feedback which I need you to integrate >> and submit new patches based upon, thanks. > > About #2: > > I still think that this: > > if (dev) > strncpy(uaddr->sa_data, dev->name, 14); > else > memset(uaddr->sa_data, 0, 14); > > is better than this: > > memset(uaddr->sa_data, 0, 14); > dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex); > if (dev) > strlcpy(uaddr->sa_data, dev->name, 15); > > Doesn't it? Explicitly filling with zero on the same "if" level is > slightly easier to read and understand. > no problem with me, since i came up with the idea a simple explanation: IMHO the pattern clear/if/copy is more robust NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15. re, wh -- 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
On Wed, Nov 10, 2010 at 12:58 +0100, walter harms wrote:
> NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15.
With this code it is NOT a bug because the output buffer is much bigger
than 14 (128 bytes). I think it was just designed to overflow 14 bytes,
assign sa_data[14] = 0 and ignore it (lack of snprintf() those days?).
Anywhere else sa_data[14] = ... is a bug.
diff --git a/net/tipc/socket.c b/net/tipc/socket.c index 33217fc..e9f0d50 100644 --- a/net/tipc/socket.c +++ b/net/tipc/socket.c @@ -396,6 +396,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr, struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr; struct tipc_sock *tsock = tipc_sk(sock->sk); + memset(addr, 0, sizeof(*addr)); if (peer) { if ((sock->state != SS_CONNECTED) && ((peer != 2) || (sock->state != SS_DISCONNECTING)))
Structure sockaddr_tipc is copied to userland with padding bytes after "id" field in union field "name" unitialized. It leads to leaking of contents of kernel stack memory. We have to initialize them to zero. Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> --- net/tipc/socket.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)