Re: [PATCHv2 net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe
From: Eric Dumazet <hidden>
Date: 2021-10-14 04:43:44
On 10/13/21 9:24 PM, Xin Long wrote:
quoted hunk ↗ jump to 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(-)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) {