* [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow @ 2010-04-06 2:50 wzt.wzt 2010-04-06 2:58 ` Changli Gao 2010-04-06 3:22 ` Simon Horman 0 siblings, 2 replies; 8+ messages in thread From: wzt.wzt @ 2010-04-06 2:50 UTC (permalink / raw) To: linux-kernel; +Cc: wensong, netdev, lvs-devel IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as: struct ip_vs_protocol ip_vs_test = { .name = "aaaaaaaa....128...aaa", .debug_packet = ip_vs_tcpudp_debug_packet, }; when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow. Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> --- net/netfilter/ipvs/ip_vs_proto.c | 16 ++++++++-------- net/netfilter/ipvs/ip_vs_proto_ah_esp.c | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c index 0e58455..8143318 100644 --- a/net/netfilter/ipvs/ip_vs_proto.c +++ b/net/netfilter/ipvs/ip_vs_proto.c @@ -166,9 +166,9 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name); else if (ih->frag_off & htons(IP_OFFSET)) - sprintf(buf, "%s %pI4->%pI4 frag", + snprintf(buf, sizeof(buf), "%s %pI4->%pI4 frag", pp->name, &ih->saddr, &ih->daddr); else { __be16 _ports[2], *pptr @@ -176,10 +176,10 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp, pptr = skb_header_pointer(skb, offset + ih->ihl*4, sizeof(_ports), _ports); if (pptr == NULL) - sprintf(buf, "%s TRUNCATED %pI4->%pI4", + snprintf(buf, sizeof(buf), "%s TRUNCATED %pI4->%pI4", pp->name, &ih->saddr, &ih->daddr); else - sprintf(buf, "%s %pI4:%u->%pI4:%u", + snprintf(buf, sizeof(buf), "%s %pI4:%u->%pI4:%u", pp->name, &ih->saddr, ntohs(pptr[0]), &ih->daddr, ntohs(pptr[1])); @@ -200,9 +200,9 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name); else if (ih->nexthdr == IPPROTO_FRAGMENT) - sprintf(buf, "%s %pI6->%pI6 frag", + snprintf(buf, sizeof(buf), "%s %pI6->%pI6 frag", pp->name, &ih->saddr, &ih->daddr); else { __be16 _ports[2], *pptr; @@ -210,10 +210,10 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp, pptr = skb_header_pointer(skb, offset + sizeof(struct ipv6hdr), sizeof(_ports), _ports); if (pptr == NULL) - sprintf(buf, "%s TRUNCATED %pI6->%pI6", + snprintf(buf, sizeof(buf), "%s TRUNCATED %pI6->%pI6", pp->name, &ih->saddr, &ih->daddr); else - sprintf(buf, "%s %pI6:%u->%pI6:%u", + snprintf(buf, sizeof(buf), "%s %pI6:%u->%pI6:%u", pp->name, &ih->saddr, ntohs(pptr[0]), &ih->daddr, ntohs(pptr[1])); diff --git a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c index c30b43c..ce795ab 100644 --- a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c +++ b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c @@ -136,9 +136,9 @@ ah_esp_debug_packet_v4(struct ip_vs_protocol *pp, const struct sk_buff *skb, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name); else - sprintf(buf, "%s %pI4->%pI4", + snprintf(buf, sizeof(buf), "%s %pI4->%pI4", pp->name, &ih->saddr, &ih->daddr); pr_debug("%s: %s\n", msg, buf); @@ -154,9 +154,9 @@ ah_esp_debug_packet_v6(struct ip_vs_protocol *pp, const struct sk_buff *skb, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name); else - sprintf(buf, "%s %pI6->%pI6", + snprintf(buf, sizeof(buf), "%s %pI6->%pI6", pp->name, &ih->saddr, &ih->daddr); pr_debug("%s: %s\n", msg, buf); -- 1.6.5.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt @ 2010-04-06 2:58 ` Changli Gao 2010-04-06 3:22 ` Simon Horman 1 sibling, 0 replies; 8+ messages in thread From: Changli Gao @ 2010-04-06 2:58 UTC (permalink / raw) To: wzt.wzt; +Cc: linux-kernel, wensong, netdev, lvs-devel On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> wrote: > IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. > struct ip_vs_protocol{} declare name as char *, if register a protocol as: > struct ip_vs_protocol ip_vs_test = { > .name = "aaaaaaaa....128...aaa", > .debug_packet = ip_vs_tcpudp_debug_packet, > }; > > when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); > will cause stack buffer overflow. > Long messages will be truncated instead of buffer overflow. We need to find a way to handle long messages elegantly. -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow @ 2010-04-06 2:58 ` Changli Gao 0 siblings, 0 replies; 8+ messages in thread From: Changli Gao @ 2010-04-06 2:58 UTC (permalink / raw) To: wzt.wzt; +Cc: linux-kernel, wensong, netdev, lvs-devel On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> wrote: > IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. > struct ip_vs_protocol{} declare name as char *, if register a protocol as: > struct ip_vs_protocol ip_vs_test = { > .name = "aaaaaaaa....128...aaa", > .debug_packet = ip_vs_tcpudp_debug_packet, > }; > > when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); > will cause stack buffer overflow. > Long messages will be truncated instead of buffer overflow. We need to find a way to handle long messages elegantly. -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-06 2:58 ` Changli Gao (?) @ 2010-04-06 3:26 ` Simon Horman -1 siblings, 0 replies; 8+ messages in thread From: Simon Horman @ 2010-04-06 3:26 UTC (permalink / raw) To: Changli Gao Cc: wzt.wzt, linux-kernel, wensong, Julian Anastasov, netdev, lvs-devel, Patrick McHardy On Tue, Apr 06, 2010 at 10:58:28AM +0800, Changli Gao wrote: > On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> wrote: > > IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. > > struct ip_vs_protocol{} declare name as char *, if register a protocol as: > > struct ip_vs_protocol ip_vs_test = { > > .name = "aaaaaaaa....128...aaa", > > .debug_packet = ip_vs_tcpudp_debug_packet, > > }; > > > > when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); > > will cause stack buffer overflow. > > > > Long messages will be truncated instead of buffer overflow. We need to > find a way to handle long messages elegantly. Its really a corner case. In practice protocol modules don't have really long names. And if one was merged that did, the buffer size could be increased at that time. So while I think its reasonable to protect against something unexpected in a protocol-module name crashing the system. Especially as that can be achieved without any real overhead. I don't think we need to sanitise the output. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt 2010-04-06 2:58 ` Changli Gao @ 2010-04-06 3:22 ` Simon Horman 2010-04-07 16:09 ` Patrick McHardy 1 sibling, 1 reply; 8+ messages in thread From: Simon Horman @ 2010-04-06 3:22 UTC (permalink / raw) To: wzt.wzt Cc: linux-kernel, Wensong Zhang, Julian Anastasov, netdev, lvs-devel, Patrick McHardy On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com wrote: > IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. > struct ip_vs_protocol{} declare name as char *, if register a protocol as: > struct ip_vs_protocol ip_vs_test = { > .name = "aaaaaaaa....128...aaa", > .debug_packet = ip_vs_tcpudp_debug_packet, > }; > > when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); > will cause stack buffer overflow. > > Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> I think that the simple answer is, don't do that. But your patch seems entirely reasonable to me. Acked-by: Simon Horman <horms@verge.net.au> Patrick, please consider merging this. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-06 3:22 ` Simon Horman @ 2010-04-07 16:09 ` Patrick McHardy 2010-04-07 22:34 ` Simon Horman 0 siblings, 1 reply; 8+ messages in thread From: Patrick McHardy @ 2010-04-07 16:09 UTC (permalink / raw) To: Simon Horman Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev, lvs-devel [-- Attachment #1: Type: text/plain, Size: 968 bytes --] Simon Horman wrote: > On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com wrote: >> IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. >> struct ip_vs_protocol{} declare name as char *, if register a protocol as: >> struct ip_vs_protocol ip_vs_test = { >> .name = "aaaaaaaa....128...aaa", >> .debug_packet = ip_vs_tcpudp_debug_packet, >> }; >> >> when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); >> will cause stack buffer overflow. >> >> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> > > I think that the simple answer is, don't do that. Indeed. > But your patch seems entirely reasonable to me. > > Acked-by: Simon Horman <horms@verge.net.au> > > Patrick, please consider merging this. I think this fix is a bit silly, we can simply print the name in the pr_debug() statement and avoid both the potential overflow and truncation. How does this look? [-- Attachment #2: x --] [-- Type: text/plain, Size: 3385 bytes --] diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c index 0e58455..27add97 100644 --- a/net/netfilter/ipvs/ip_vs_proto.c +++ b/net/netfilter/ipvs/ip_vs_proto.c @@ -166,26 +166,24 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + sprintf(buf, "TRUNCATED"); else if (ih->frag_off & htons(IP_OFFSET)) - sprintf(buf, "%s %pI4->%pI4 frag", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "%pI4->%pI4 frag", &ih->saddr, &ih->daddr); else { __be16 _ports[2], *pptr ; pptr = skb_header_pointer(skb, offset + ih->ihl*4, sizeof(_ports), _ports); if (pptr == NULL) - sprintf(buf, "%s TRUNCATED %pI4->%pI4", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "TRUNCATED %pI4->%pI4", + &ih->saddr, &ih->daddr); else - sprintf(buf, "%s %pI4:%u->%pI4:%u", - pp->name, + sprintf(buf, "%pI4:%u->%pI4:%u", &ih->saddr, ntohs(pptr[0]), &ih->daddr, ntohs(pptr[1])); } - pr_debug("%s: %s\n", msg, buf); + pr_debug("%s: %s %s\n", msg, pp->name, buf); } #ifdef CONFIG_IP_VS_IPV6 @@ -200,26 +198,24 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + sprintf(buf, "TRUNCATED"); else if (ih->nexthdr == IPPROTO_FRAGMENT) - sprintf(buf, "%s %pI6->%pI6 frag", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "%pI6->%pI6 frag", &ih->saddr, &ih->daddr); else { __be16 _ports[2], *pptr; pptr = skb_header_pointer(skb, offset + sizeof(struct ipv6hdr), sizeof(_ports), _ports); if (pptr == NULL) - sprintf(buf, "%s TRUNCATED %pI6->%pI6", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "TRUNCATED %pI6->%pI6", + &ih->saddr, &ih->daddr); else - sprintf(buf, "%s %pI6:%u->%pI6:%u", - pp->name, + sprintf(buf, "%pI6:%u->%pI6:%u", &ih->saddr, ntohs(pptr[0]), &ih->daddr, ntohs(pptr[1])); } - pr_debug("%s: %s\n", msg, buf); + pr_debug("%s: %s %s\n", msg, pp->name, buf); } #endif diff --git a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c index c30b43c..1892dfc 100644 --- a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c +++ b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c @@ -136,12 +136,11 @@ ah_esp_debug_packet_v4(struct ip_vs_protocol *pp, const struct sk_buff *skb, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + sprintf(buf, "TRUNCATED"); else - sprintf(buf, "%s %pI4->%pI4", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "%pI4->%pI4", &ih->saddr, &ih->daddr); - pr_debug("%s: %s\n", msg, buf); + pr_debug("%s: %s %s\n", msg, pp->name, buf); } #ifdef CONFIG_IP_VS_IPV6 @@ -154,12 +153,11 @@ ah_esp_debug_packet_v6(struct ip_vs_protocol *pp, const struct sk_buff *skb, ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph); if (ih == NULL) - sprintf(buf, "%s TRUNCATED", pp->name); + sprintf(buf, "TRUNCATED"); else - sprintf(buf, "%s %pI6->%pI6", - pp->name, &ih->saddr, &ih->daddr); + sprintf(buf, "%pI6->%pI6", &ih->saddr, &ih->daddr); - pr_debug("%s: %s\n", msg, buf); + pr_debug("%s: %s %s\n", msg, pp->name, buf); } #endif ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-07 16:09 ` Patrick McHardy @ 2010-04-07 22:34 ` Simon Horman 2010-04-08 11:37 ` Patrick McHardy 0 siblings, 1 reply; 8+ messages in thread From: Simon Horman @ 2010-04-07 22:34 UTC (permalink / raw) To: Patrick McHardy Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev, lvs-devel On Wed, Apr 07, 2010 at 06:09:54PM +0200, Patrick McHardy wrote: > Simon Horman wrote: > > On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com wrote: > >> IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. > >> struct ip_vs_protocol{} declare name as char *, if register a protocol as: > >> struct ip_vs_protocol ip_vs_test = { > >> .name = "aaaaaaaa....128...aaa", > >> .debug_packet = ip_vs_tcpudp_debug_packet, > >> }; > >> > >> when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); > >> will cause stack buffer overflow. > >> > >> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> > > > > I think that the simple answer is, don't do that. > > Indeed. > > > But your patch seems entirely reasonable to me. > > > > Acked-by: Simon Horman <horms@verge.net.au> > > > > Patrick, please consider merging this. > > I think this fix is a bit silly, we can simply print the name in > the pr_debug() statement and avoid both the potential overflow > and truncation. > > How does this look? Looks good to me: Acked-by: Simon Horman <horms@verge.net.au> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow 2010-04-07 22:34 ` Simon Horman @ 2010-04-08 11:37 ` Patrick McHardy 0 siblings, 0 replies; 8+ messages in thread From: Patrick McHardy @ 2010-04-08 11:37 UTC (permalink / raw) To: Simon Horman Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev, lvs-devel Simon Horman wrote: > On Wed, Apr 07, 2010 at 06:09:54PM +0200, Patrick McHardy wrote: >> Simon Horman wrote: >>> On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com wrote: >>>> IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. >>>> struct ip_vs_protocol{} declare name as char *, if register a protocol as: >>>> struct ip_vs_protocol ip_vs_test = { >>>> .name = "aaaaaaaa....128...aaa", >>>> .debug_packet = ip_vs_tcpudp_debug_packet, >>>> }; >>>> >>>> when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); >>>> will cause stack buffer overflow. >>>> >>>> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> >>> I think that the simple answer is, don't do that. >> Indeed. >> >>> But your patch seems entirely reasonable to me. >>> >>> Acked-by: Simon Horman <horms@verge.net.au> >>> >>> Patrick, please consider merging this. >> I think this fix is a bit silly, we can simply print the name in >> the pr_debug() statement and avoid both the potential overflow >> and truncation. >> >> How does this look? > > Looks good to me: > > Acked-by: Simon Horman <horms@verge.net.au> Thanks, I've applied the patch. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-04-08 11:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt 2010-04-06 2:58 ` Changli Gao 2010-04-06 2:58 ` Changli Gao 2010-04-06 3:26 ` Simon Horman 2010-04-06 3:22 ` Simon Horman 2010-04-07 16:09 ` Patrick McHardy 2010-04-07 22:34 ` Simon Horman 2010-04-08 11:37 ` Patrick McHardy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.