| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202505251717.YSYmRdLZ-lkp@intel.com>
Date: Mon, 26 May 2025 09:36:41 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: oe-kbuild@...ts.linux.dev, Rafal Bilkowski <rafalbilkowski@...il.com>,
netdev@...r.kernel.org
Cc: lkp@...el.com, oe-kbuild-all@...ts.linux.dev,
linux-kernel@...r.kernel.org, dsahern@...nel.org,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
Rafal Bilkowski <rafalbilkowski@...il.com>
Subject: Re: [PATCH] net: ipv6: sanitize RPL SRH cmpre/cmpre fields to fix
taint issue
Hi Rafal,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Rafal-Bilkowski/net-ipv6-sanitize-RPL-SRH-cmpre-cmpre-fields-to-fix-taint-issue/20250524-135351
base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
patch link: https://lore.kernel.org/r/20250524055159.32982-1-rafalbilkowski%40gmail.com
patch subject: [PATCH] net: ipv6: sanitize RPL SRH cmpre/cmpre fields to fix taint issue
config: parisc-randconfig-r072-20250525 (https://download.01.org/0day-ci/archive/20250525/202505251717.YSYmRdLZ-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.4.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@...el.com>
| Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
| Closes: https://lore.kernel.org/r/202505251717.YSYmRdLZ-lkp@intel.com/
smatch warnings:
net/ipv6/exthdrs.c:511 ipv6_rpl_srh_rcv() error: uninitialized symbol 'hdr'.
net/ipv6/exthdrs.c:547 ipv6_rpl_srh_rcv() warn: impossible condition '(hdr->cmpri > 15) => (0-15 > 15)'
net/ipv6/exthdrs.c:547 ipv6_rpl_srh_rcv() warn: impossible condition '(hdr->cmpre > 15) => (0-15 > 15)'
net/ipv6/exthdrs.c:550 ipv6_rpl_srh_rcv() warn: impossible condition '(hdr->pad > 15) => (0-15 > 15)'
net/ipv6/exthdrs.c:555 ipv6_rpl_srh_rcv() warn: 'n' unsigned <= 0
vim +/hdr +511 net/ipv6/exthdrs.c
8610c7c6e3bd64 Alexander Aring 2020-03-27 482 static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
8610c7c6e3bd64 Alexander Aring 2020-03-27 483 {
8610c7c6e3bd64 Alexander Aring 2020-03-27 484 struct ipv6_rpl_sr_hdr *hdr, *ohdr, *chdr;
8610c7c6e3bd64 Alexander Aring 2020-03-27 485 struct inet6_skb_parm *opt = IP6CB(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 486 struct net *net = dev_net(skb->dev);
8610c7c6e3bd64 Alexander Aring 2020-03-27 487 struct inet6_dev *idev;
8610c7c6e3bd64 Alexander Aring 2020-03-27 488 struct ipv6hdr *oldhdr;
8610c7c6e3bd64 Alexander Aring 2020-03-27 489 unsigned char *buf;
8610c7c6e3bd64 Alexander Aring 2020-03-27 490 int accept_rpl_seg;
8610c7c6e3bd64 Alexander Aring 2020-03-27 491 int i, err;
8610c7c6e3bd64 Alexander Aring 2020-03-27 492 u64 n = 0;
8610c7c6e3bd64 Alexander Aring 2020-03-27 493 u32 r;
8610c7c6e3bd64 Alexander Aring 2020-03-27 494
8610c7c6e3bd64 Alexander Aring 2020-03-27 495 idev = __in6_dev_get(skb->dev);
8610c7c6e3bd64 Alexander Aring 2020-03-27 496
8610c7c6e3bd64 Alexander Aring 2020-03-27 497 accept_rpl_seg = net->ipv6.devconf_all->rpl_seg_enabled;
8610c7c6e3bd64 Alexander Aring 2020-03-27 498 if (accept_rpl_seg > idev->cnf.rpl_seg_enabled)
8610c7c6e3bd64 Alexander Aring 2020-03-27 499 accept_rpl_seg = idev->cnf.rpl_seg_enabled;
8610c7c6e3bd64 Alexander Aring 2020-03-27 500
8610c7c6e3bd64 Alexander Aring 2020-03-27 501 if (!accept_rpl_seg) {
8610c7c6e3bd64 Alexander Aring 2020-03-27 502 kfree_skb(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 503 return -1;
8610c7c6e3bd64 Alexander Aring 2020-03-27 504 }
8610c7c6e3bd64 Alexander Aring 2020-03-27 505
8610c7c6e3bd64 Alexander Aring 2020-03-27 506 looped_back:
045bd21f380952 Rafal Bilkowski 2025-05-24 507
045bd21f380952 Rafal Bilkowski 2025-05-24 508 if (!pskb_may_pull(skb, skb_transport_offset(skb) + sizeof(struct ipv6_rpl_sr_hdr)))
045bd21f380952 Rafal Bilkowski 2025-05-24 509 goto error;
045bd21f380952 Rafal Bilkowski 2025-05-24 510 // Check if there is enough memory available for the header and hdrlen is in valid range
045bd21f380952 Rafal Bilkowski 2025-05-24 @511 if (skb_tailroom(skb) < ((hdr->hdrlen + 1) << 3) ||
Where is hdr initialized? Is part of the commit missing?
045bd21f380952 Rafal Bilkowski 2025-05-24 512 hdr->hdrlen == 0 ||
045bd21f380952 Rafal Bilkowski 2025-05-24 513 hdr->hdrlen > U8_MAX)
045bd21f380952 Rafal Bilkowski 2025-05-24 514 goto error;
045bd21f380952 Rafal Bilkowski 2025-05-24 515
8610c7c6e3bd64 Alexander Aring 2020-03-27 516 hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 517
8610c7c6e3bd64 Alexander Aring 2020-03-27 518 if (hdr->segments_left == 0) {
8610c7c6e3bd64 Alexander Aring 2020-03-27 519 if (hdr->nexthdr == NEXTHDR_IPV6) {
8610c7c6e3bd64 Alexander Aring 2020-03-27 520 int offset = (hdr->hdrlen + 1) << 3;
8610c7c6e3bd64 Alexander Aring 2020-03-27 521
8610c7c6e3bd64 Alexander Aring 2020-03-27 522 skb_postpull_rcsum(skb, skb_network_header(skb),
8610c7c6e3bd64 Alexander Aring 2020-03-27 523 skb_network_header_len(skb));
ac9d8a66e41d55 Kuniyuki Iwashima 2023-06-14 524 skb_pull(skb, offset);
8610c7c6e3bd64 Alexander Aring 2020-03-27 525 skb_postpull_rcsum(skb, skb_transport_header(skb),
8610c7c6e3bd64 Alexander Aring 2020-03-27 526 offset);
8610c7c6e3bd64 Alexander Aring 2020-03-27 527
8610c7c6e3bd64 Alexander Aring 2020-03-27 528 skb_reset_network_header(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 529 skb_reset_transport_header(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 530 skb->encapsulation = 0;
8610c7c6e3bd64 Alexander Aring 2020-03-27 531
8610c7c6e3bd64 Alexander Aring 2020-03-27 532 __skb_tunnel_rx(skb, skb->dev, net);
8610c7c6e3bd64 Alexander Aring 2020-03-27 533
8610c7c6e3bd64 Alexander Aring 2020-03-27 534 netif_rx(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 535 return -1;
8610c7c6e3bd64 Alexander Aring 2020-03-27 536 }
8610c7c6e3bd64 Alexander Aring 2020-03-27 537
8610c7c6e3bd64 Alexander Aring 2020-03-27 538 opt->srcrt = skb_network_header_len(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 539 opt->lastopt = opt->srcrt;
8610c7c6e3bd64 Alexander Aring 2020-03-27 540 skb->transport_header += (hdr->hdrlen + 1) << 3;
8610c7c6e3bd64 Alexander Aring 2020-03-27 541 opt->nhoff = (&hdr->nexthdr) - skb_network_header(skb);
8610c7c6e3bd64 Alexander Aring 2020-03-27 542
8610c7c6e3bd64 Alexander Aring 2020-03-27 543 return 1;
8610c7c6e3bd64 Alexander Aring 2020-03-27 544 }
8610c7c6e3bd64 Alexander Aring 2020-03-27 545
045bd21f380952 Rafal Bilkowski 2025-05-24 546 // Check if cmpri and cmpre are valid and do not exceed 15
These comments are pointless. It's just explaining what an if statement
does but it doesn't explain why 15 is chosen. #MagicNumber
045bd21f380952 Rafal Bilkowski 2025-05-24 @547 if (hdr->cmpri > 15 || hdr->cmpre > 15)
045bd21f380952 Rafal Bilkowski 2025-05-24 548 goto error;
I don't normally report these because it's fine to add pointless
if statements if it makes the code more readable. But that's
not really what's happening here based on the commit message.
045bd21f380952 Rafal Bilkowski 2025-05-24 549 // Check if pad value is valid and does not exceed 15
045bd21f380952 Rafal Bilkowski 2025-05-24 @550 if (hdr->pad > 15)
045bd21f380952 Rafal Bilkowski 2025-05-24 551 goto error;
045bd21f380952 Rafal Bilkowski 2025-05-24 552
8610c7c6e3bd64 Alexander Aring 2020-03-27 553 n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
045bd21f380952 Rafal Bilkowski 2025-05-24 554 // Check if n is non-negative
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
n is unsigned so it can't be negative.
045bd21f380952 Rafal Bilkowski 2025-05-24 @555 if (n <= 0)
045bd21f380952 Rafal Bilkowski 2025-05-24 556 goto error;
045bd21f380952 Rafal Bilkowski 2025-05-24 557
8610c7c6e3bd64 Alexander Aring 2020-03-27 558 r = do_div(n, (16 - hdr->cmpri));
8610c7c6e3bd64 Alexander Aring 2020-03-27 559 /* checks if calculation was without remainder and n fits into
8610c7c6e3bd64 Alexander Aring 2020-03-27 560 * unsigned char which is segments_left field. Should not be
8610c7c6e3bd64 Alexander Aring 2020-03-27 561 * higher than that.
8610c7c6e3bd64 Alexander Aring 2020-03-27 562 */
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Powered by blists - more mailing lists