From: Xin Long <lucien.xin@gmail.com> Date: 2021-10-14 04:25:01
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 <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv4/icmp.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
From: Eric Dumazet <hidden> Date: 2021-10-14 04:43:44
On 10/13/21 9:24 PM, Xin Long wrote:
quoted hunk
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 <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv4/icmp.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
??? 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) {
From: Xin Long <lucien.xin@gmail.com> Date: 2021-10-14 09:35:37
On Thu, Oct 14, 2021 at 12:43 PM Eric Dumazet [off-list ref] wrote:
On 10/13/21 9:24 PM, Xin Long wrote:
quoted
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 <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv4/icmp.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
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.
quoted
+ if (!iio)
+ goto send_mal_query;
+
status = 0;
dev = NULL;
switch (iio->extobj_hdr.class_type) {