| Message ID | 345b3f75bea482f7b3174297261db24cdf7e15e1.1634185497.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2,net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe | expand |
On 10/13/21 9:24 PM, Xin Long wrote: > In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done > step by step and skb_header_pointer() return value should always be > checked, this patch fixes 3 places in there: > > - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name > from skb by skb_header_pointer(), its len is ident_len. Besides, > the return value of skb_header_pointer() should always be checked. > > - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of > skb_header_pointer(), and also do the return value check for > skb_header_pointer(). > > - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr. > ctype3_hdr.addrlen, skb_header_pointer() should be called first, > then check its return value and ident_len. > On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident. > addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value. > On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be > "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) + > sizeof(struct in_addr)" or "ident_len". > > v1->v2: > - To make it more clear, call skb_header_pointer() once only for > iio->indent's parsing as Jakub Suggested. > > Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/ipv4/icmp.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index 8b30cadff708..bccb2132a464 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) > if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr)) > goto send_mal_query; > ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr); > + iio = skb_header_pointer(skb, sizeof(_ext_hdr), > + sizeof(iio->extobj_hdr) + ident_len, &_iio); ??? How has this been tested ??? If you pass &_iio for last argument, then you _must_ use sizeof(__iio) (or smaller) too for third argument, or risk stack overflow, in the case page frag bytes need to be copied into _iio If the remote peer cooks a malicious packet so that ident_len is big like 1200, then for sure the kernel will crash, because sizeof(iio->extobj_hdr) + ident_len will be bigger than sizeof(_iio) > + if (!iio) > + goto send_mal_query; > + > status = 0; > dev = NULL; > switch (iio->extobj_hdr.class_type) {
On Thu, Oct 14, 2021 at 12:43 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 10/13/21 9:24 PM, Xin Long wrote: > > In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done > > step by step and skb_header_pointer() return value should always be > > checked, this patch fixes 3 places in there: > > > > - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name > > from skb by skb_header_pointer(), its len is ident_len. Besides, > > the return value of skb_header_pointer() should always be checked. > > > > - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of > > skb_header_pointer(), and also do the return value check for > > skb_header_pointer(). > > > > - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr. > > ctype3_hdr.addrlen, skb_header_pointer() should be called first, > > then check its return value and ident_len. > > On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident. > > addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value. > > On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be > > "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) + > > sizeof(struct in_addr)" or "ident_len". > > > > v1->v2: > > - To make it more clear, call skb_header_pointer() once only for > > iio->indent's parsing as Jakub Suggested. > > > > Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages") > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/ipv4/icmp.c | 20 +++++++++----------- > > 1 file changed, 9 insertions(+), 11 deletions(-) > > > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > > index 8b30cadff708..bccb2132a464 100644 > > --- a/net/ipv4/icmp.c > > +++ b/net/ipv4/icmp.c > > @@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) > > if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr)) > > goto send_mal_query; > > ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr); > > + iio = skb_header_pointer(skb, sizeof(_ext_hdr), > > + sizeof(iio->extobj_hdr) + ident_len, &_iio); > > ??? How has this been tested ??? This actually is difficult to cook a non-linear skb to be tested. In the testing, if it's a linear skb, I realized it won't use &_iio memory. when the value was greater than skb's len, it returned NULL. when the value was less than skb's len, it just used skb->data memory. > > If you pass &_iio for last argument, then you _must_ use sizeof(__iio) (or smaller) too for third argument, > or risk stack overflow, in the case page frag bytes need to be copied into _iio > > If the remote peer cooks a malicious packet so that ident_len is big like 1200, > then for sure the kernel will crash, > because sizeof(iio->extobj_hdr) + ident_len will be bigger than sizeof(_iio) You're right, more check is needed before calling skb_header_pointer(): - if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr)) + if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr) || + ntohs(iio->extobj_hdr.length) > sizeof(_iio)) goto send_mal_query; ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr); iio = skb_header_pointer(skb, sizeof(_ext_hdr), Thanks. > > > > + if (!iio) > > + goto send_mal_query; > > + > > status = 0; > > dev = NULL; > > switch (iio->extobj_hdr.class_type) {
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 8b30cadff708..bccb2132a464 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr)) goto send_mal_query; ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr); + iio = skb_header_pointer(skb, sizeof(_ext_hdr), + sizeof(iio->extobj_hdr) + ident_len, &_iio); + if (!iio) + goto send_mal_query; + status = 0; dev = NULL; switch (iio->extobj_hdr.class_type) { case ICMP_EXT_ECHO_CTYPE_NAME: - iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio); if (ident_len >= IFNAMSIZ) goto send_mal_query; memset(buff, 0, sizeof(buff)); @@ -1069,30 +1073,24 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) dev = dev_get_by_name(net, buff); break; case ICMP_EXT_ECHO_CTYPE_INDEX: - iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + - sizeof(iio->ident.ifindex), &_iio); if (ident_len != sizeof(iio->ident.ifindex)) goto send_mal_query; dev = dev_get_by_index(net, ntohl(iio->ident.ifindex)); break; case ICMP_EXT_ECHO_CTYPE_ADDR: - if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) + + if (ident_len < sizeof(iio->ident.addr.ctype3_hdr) || + ident_len != sizeof(iio->ident.addr.ctype3_hdr) + iio->ident.addr.ctype3_hdr.addrlen) goto send_mal_query; switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) { case ICMP_AFI_IP: - iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + - sizeof(struct in_addr), &_iio); - if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) + - sizeof(struct in_addr)) + if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr)) goto send_mal_query; dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr); break; #if IS_ENABLED(CONFIG_IPV6) case ICMP_AFI_IP6: - iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio); - if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) + - sizeof(struct in6_addr)) + if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr)) goto send_mal_query; dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev); dev_hold(dev);
In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done step by step and skb_header_pointer() return value should always be checked, this patch fixes 3 places in there: - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name from skb by skb_header_pointer(), its len is ident_len. Besides, the return value of skb_header_pointer() should always be checked. - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of skb_header_pointer(), and also do the return value check for skb_header_pointer(). - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr. ctype3_hdr.addrlen, skb_header_pointer() should be called first, then check its return value and ident_len. On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident. addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value. On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) + sizeof(struct in_addr)" or "ident_len". v1->v2: - To make it more clear, call skb_header_pointer() once only for iio->indent's parsing as Jakub Suggested. Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/ipv4/icmp.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)