* Re: [bug report] lsm: replace context+len with lsm_context
[not found] <549fd50d-a631-4103-bfe2-e842de387163@stanley.mountain>
@ 2024-11-05 11:12 ` Pablo Neira Ayuso
2024-11-05 20:09 ` Casey Schaufler
0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-05 11:12 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Dan Carpenter, netfilter-devel, Paul Moore, LSM List
Hi,
There is another issue in this recent series, see below.
This needs another follow up.
Thanks.
On Sat, Nov 02, 2024 at 12:21:01PM +0300, Dan Carpenter wrote:
> Hello Casey Schaufler,
>
> Commit 95a3c11eb670 ("lsm: replace context+len with lsm_context")
> from Oct 23, 2024 (linux-next), leads to the following Smatch static
> checker warning:
>
> net/netfilter/nfnetlink_queue.c:646 nfqnl_build_packet_message()
> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>
> net/netfilter/nfnetlink_queue.c:813 nfqnl_build_packet_message()
> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>
> net/netfilter/nfnetlink_queue.c:822 nfqnl_build_packet_message()
> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>
> net/netfilter/nfnetlink_queue.c
> 551 static struct sk_buff *
> 552 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> 553 struct nf_queue_entry *entry,
> 554 __be32 **packet_id_ptr)
> 555 {
> 556 size_t size;
> 557 size_t data_len = 0, cap_len = 0;
> 558 unsigned int hlen = 0;
> 559 struct sk_buff *skb;
> 560 struct nlattr *nla;
> 561 struct nfqnl_msg_packet_hdr *pmsg;
> 562 struct nlmsghdr *nlh;
> 563 struct sk_buff *entskb = entry->skb;
> 564 struct net_device *indev;
> 565 struct net_device *outdev;
> 566 struct nf_conn *ct = NULL;
> 567 enum ip_conntrack_info ctinfo = 0;
> 568 const struct nfnl_ct_hook *nfnl_ct;
> 569 bool csum_verify;
> 570 struct lsm_context ctx;
> 571 u32 seclen = 0;
> ^^^^^^^^^^
>
> 572 ktime_t tstamp;
> 573
> 574 size = nlmsg_total_size(sizeof(struct nfgenmsg))
> 575 + nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
> 576 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> 577 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> 578 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> 579 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> 580 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> 581 #endif
> 582 + nla_total_size(sizeof(u_int32_t)) /* mark */
> 583 + nla_total_size(sizeof(u_int32_t)) /* priority */
> 584 + nla_total_size(sizeof(struct nfqnl_msg_packet_hw))
> 585 + nla_total_size(sizeof(u_int32_t)) /* skbinfo */
> 586 #if IS_ENABLED(CONFIG_CGROUP_NET_CLASSID)
> 587 + nla_total_size(sizeof(u_int32_t)) /* classid */
> 588 #endif
> 589 + nla_total_size(sizeof(u_int32_t)); /* cap_len */
> 590
> 591 tstamp = skb_tstamp_cond(entskb, false);
> 592 if (tstamp)
> 593 size += nla_total_size(sizeof(struct nfqnl_msg_packet_timestamp));
> 594
> 595 size += nfqnl_get_bridge_size(entry);
> 596
> 597 if (entry->state.hook <= NF_INET_FORWARD ||
> 598 (entry->state.hook == NF_INET_POST_ROUTING && entskb->sk == NULL))
> 599 csum_verify = !skb_csum_unnecessary(entskb);
> 600 else
> 601 csum_verify = false;
> 602
> 603 outdev = entry->state.out;
> 604
> 605 switch ((enum nfqnl_config_mode)READ_ONCE(queue->copy_mode)) {
> 606 case NFQNL_COPY_META:
> 607 case NFQNL_COPY_NONE:
> 608 break;
> 609
> 610 case NFQNL_COPY_PACKET:
> 611 if (!(queue->flags & NFQA_CFG_F_GSO) &&
> 612 entskb->ip_summed == CHECKSUM_PARTIAL &&
> 613 nf_queue_checksum_help(entskb))
> 614 return NULL;
> 615
> 616 data_len = READ_ONCE(queue->copy_range);
> 617 if (data_len > entskb->len)
> 618 data_len = entskb->len;
> 619
> 620 hlen = skb_zerocopy_headlen(entskb);
> 621 hlen = min_t(unsigned int, hlen, data_len);
> 622 size += sizeof(struct nlattr) + hlen;
> 623 cap_len = entskb->len;
> 624 break;
> 625 }
> 626
> 627 nfnl_ct = rcu_dereference(nfnl_ct_hook);
> 628
> 629 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> 630 if (queue->flags & NFQA_CFG_F_CONNTRACK) {
> 631 if (nfnl_ct != NULL) {
> 632 ct = nf_ct_get(entskb, &ctinfo);
> 633 if (ct != NULL)
> 634 size += nfnl_ct->build_size(ct);
> 635 }
> 636 }
> 637 #endif
> 638
> 639 if (queue->flags & NFQA_CFG_F_UID_GID) {
> 640 size += (nla_total_size(sizeof(u_int32_t)) /* uid */
> 641 + nla_total_size(sizeof(u_int32_t))); /* gid */
> 642 }
> 643
> 644 if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> 645 seclen = nfqnl_get_sk_secctx(entskb, &ctx);
>
> nfqnl_get_sk_secctx() returns negative error codes. It needs to be changed ot
> int as well instead of u32.
>
> 646 if (seclen >= 0)
> 647 size += nla_total_size(seclen);
> 648 }
> 649
> 650 skb = alloc_skb(size, GFP_ATOMIC);
> 651 if (!skb) {
> 652 skb_tx_error(entskb);
> 653 goto nlmsg_failure;
> 654 }
> 655
> 656 nlh = nfnl_msg_put(skb, 0, 0,
> 657 nfnl_msg_type(NFNL_SUBSYS_QUEUE, NFQNL_MSG_PACKET),
> 658 0, entry->state.pf, NFNETLINK_V0,
> 659 htons(queue->queue_num));
> 660 if (!nlh) {
> 661 skb_tx_error(entskb);
> 662 kfree_skb(skb);
> 663 goto nlmsg_failure;
> 664 }
> 665
> 666 nla = __nla_reserve(skb, NFQA_PACKET_HDR, sizeof(*pmsg));
> 667 pmsg = nla_data(nla);
> 668 pmsg->hw_protocol = entskb->protocol;
> 669 pmsg->hook = entry->state.hook;
> 670 *packet_id_ptr = &pmsg->packet_id;
> 671
> 672 indev = entry->state.in;
> 673 if (indev) {
> 674 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> 675 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV, htonl(indev->ifindex)))
> 676 goto nla_put_failure;
> 677 #else
> 678 if (entry->state.pf == PF_BRIDGE) {
> 679 /* Case 1: indev is physical input device, we need to
> 680 * look for bridge group (when called from
> 681 * netfilter_bridge) */
> 682 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
> 683 htonl(indev->ifindex)) ||
> 684 /* this is the bridge group "brX" */
> 685 /* rcu_read_lock()ed by __nf_queue */
> 686 nla_put_be32(skb, NFQA_IFINDEX_INDEV,
> 687 htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
> 688 goto nla_put_failure;
> 689 } else {
> 690 int physinif;
> 691
> 692 /* Case 2: indev is bridge group, we need to look for
> 693 * physical device (when called from ipv4) */
> 694 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV,
> 695 htonl(indev->ifindex)))
> 696 goto nla_put_failure;
> 697
> 698 physinif = nf_bridge_get_physinif(entskb);
> 699 if (physinif &&
> 700 nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
> 701 htonl(physinif)))
> 702 goto nla_put_failure;
> 703 }
> 704 #endif
> 705 }
> 706
> 707 if (outdev) {
> 708 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> 709 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV, htonl(outdev->ifindex)))
> 710 goto nla_put_failure;
> 711 #else
> 712 if (entry->state.pf == PF_BRIDGE) {
> 713 /* Case 1: outdev is physical output device, we need to
> 714 * look for bridge group (when called from
> 715 * netfilter_bridge) */
> 716 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
> 717 htonl(outdev->ifindex)) ||
> 718 /* this is the bridge group "brX" */
> 719 /* rcu_read_lock()ed by __nf_queue */
> 720 nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
> 721 htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
> 722 goto nla_put_failure;
> 723 } else {
> 724 int physoutif;
> 725
> 726 /* Case 2: outdev is bridge group, we need to look for
> 727 * physical output device (when called from ipv4) */
> 728 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
> 729 htonl(outdev->ifindex)))
> 730 goto nla_put_failure;
> 731
> 732 physoutif = nf_bridge_get_physoutif(entskb);
> 733 if (physoutif &&
> 734 nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
> 735 htonl(physoutif)))
> 736 goto nla_put_failure;
> 737 }
> 738 #endif
> 739 }
> 740
> 741 if (entskb->mark &&
> 742 nla_put_be32(skb, NFQA_MARK, htonl(entskb->mark)))
> 743 goto nla_put_failure;
> 744
> 745 if (entskb->priority &&
> 746 nla_put_be32(skb, NFQA_PRIORITY, htonl(entskb->priority)))
> 747 goto nla_put_failure;
> 748
> 749 if (indev && entskb->dev &&
> 750 skb_mac_header_was_set(entskb) &&
> 751 skb_mac_header_len(entskb) != 0) {
> 752 struct nfqnl_msg_packet_hw phw;
> 753 int len;
> 754
> 755 memset(&phw, 0, sizeof(phw));
> 756 len = dev_parse_header(entskb, phw.hw_addr);
> 757 if (len) {
> 758 phw.hw_addrlen = htons(len);
> 759 if (nla_put(skb, NFQA_HWADDR, sizeof(phw), &phw))
> 760 goto nla_put_failure;
> 761 }
> 762 }
> 763
> 764 if (nfqnl_put_bridge(entry, skb) < 0)
> 765 goto nla_put_failure;
> 766
> 767 if (entry->state.hook <= NF_INET_FORWARD && tstamp) {
> 768 struct nfqnl_msg_packet_timestamp ts;
> 769 struct timespec64 kts = ktime_to_timespec64(tstamp);
> 770
> 771 ts.sec = cpu_to_be64(kts.tv_sec);
> 772 ts.usec = cpu_to_be64(kts.tv_nsec / NSEC_PER_USEC);
> 773
> 774 if (nla_put(skb, NFQA_TIMESTAMP, sizeof(ts), &ts))
> 775 goto nla_put_failure;
> 776 }
> 777
> 778 if ((queue->flags & NFQA_CFG_F_UID_GID) && entskb->sk &&
> 779 nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
> 780 goto nla_put_failure;
> 781
> 782 if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
> 783 goto nla_put_failure;
> 784
> 785 if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
> ^^^^^^
> This doesn't look right.
>
>
> 786 goto nla_put_failure;
> 787
> 788 if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> 789 goto nla_put_failure;
> 790
> 791 if (cap_len > data_len &&
> 792 nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
> 793 goto nla_put_failure;
> 794
> 795 if (nfqnl_put_packet_info(skb, entskb, csum_verify))
> 796 goto nla_put_failure;
> 797
> 798 if (data_len) {
> 799 struct nlattr *nla;
> 800
> 801 if (skb_tailroom(skb) < sizeof(*nla) + hlen)
> 802 goto nla_put_failure;
> 803
> 804 nla = skb_put(skb, sizeof(*nla));
> 805 nla->nla_type = NFQA_PAYLOAD;
> 806 nla->nla_len = nla_attr_size(data_len);
> 807
> 808 if (skb_zerocopy(skb, entskb, data_len, hlen))
> 809 goto nla_put_failure;
> 810 }
> 811
> 812 nlh->nlmsg_len = skb->len;
> 813 if (seclen >= 0)
> ^^^^^^^^^^^
>
> 814 security_release_secctx(&ctx);
> 815 return skb;
> 816
> 817 nla_put_failure:
> 818 skb_tx_error(entskb);
> 819 kfree_skb(skb);
> 820 net_err_ratelimited("nf_queue: error creating packet message\n");
> 821 nlmsg_failure:
> 822 if (seclen >= 0)
> ^^^^^^^^^^^
>
> 823 security_release_secctx(&ctx);
> 824 return NULL;
> 825 }
>
>
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] lsm: replace context+len with lsm_context
2024-11-05 11:12 ` [bug report] lsm: replace context+len with lsm_context Pablo Neira Ayuso
@ 2024-11-05 20:09 ` Casey Schaufler
2024-11-05 21:29 ` Paul Moore
2024-11-06 10:10 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Casey Schaufler @ 2024-11-05 20:09 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Dan Carpenter, netfilter-devel, Paul Moore, LSM List,
Casey Schaufler
On 11/5/2024 3:12 AM, Pablo Neira Ayuso wrote:
> Hi,
>
> There is another issue in this recent series, see below.
>
> This needs another follow up.
Dan Carpenter sent a patch:
https://lore.kernel.org/lkml/b226a01a-2545-4b67-9cc6-59cfa0ffabbc@schaufler-ca.com/
>
> Thanks.
>
> On Sat, Nov 02, 2024 at 12:21:01PM +0300, Dan Carpenter wrote:
>> Hello Casey Schaufler,
>>
>> Commit 95a3c11eb670 ("lsm: replace context+len with lsm_context")
>> from Oct 23, 2024 (linux-next), leads to the following Smatch static
>> checker warning:
>>
>> net/netfilter/nfnetlink_queue.c:646 nfqnl_build_packet_message()
>> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>>
>> net/netfilter/nfnetlink_queue.c:813 nfqnl_build_packet_message()
>> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>>
>> net/netfilter/nfnetlink_queue.c:822 nfqnl_build_packet_message()
>> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
>>
>> net/netfilter/nfnetlink_queue.c
>> 551 static struct sk_buff *
>> 552 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> 553 struct nf_queue_entry *entry,
>> 554 __be32 **packet_id_ptr)
>> 555 {
>> 556 size_t size;
>> 557 size_t data_len = 0, cap_len = 0;
>> 558 unsigned int hlen = 0;
>> 559 struct sk_buff *skb;
>> 560 struct nlattr *nla;
>> 561 struct nfqnl_msg_packet_hdr *pmsg;
>> 562 struct nlmsghdr *nlh;
>> 563 struct sk_buff *entskb = entry->skb;
>> 564 struct net_device *indev;
>> 565 struct net_device *outdev;
>> 566 struct nf_conn *ct = NULL;
>> 567 enum ip_conntrack_info ctinfo = 0;
>> 568 const struct nfnl_ct_hook *nfnl_ct;
>> 569 bool csum_verify;
>> 570 struct lsm_context ctx;
>> 571 u32 seclen = 0;
>> ^^^^^^^^^^
>>
>> 572 ktime_t tstamp;
>> 573
>> 574 size = nlmsg_total_size(sizeof(struct nfgenmsg))
>> 575 + nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
>> 576 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
>> 577 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
>> 578 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>> 579 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
>> 580 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
>> 581 #endif
>> 582 + nla_total_size(sizeof(u_int32_t)) /* mark */
>> 583 + nla_total_size(sizeof(u_int32_t)) /* priority */
>> 584 + nla_total_size(sizeof(struct nfqnl_msg_packet_hw))
>> 585 + nla_total_size(sizeof(u_int32_t)) /* skbinfo */
>> 586 #if IS_ENABLED(CONFIG_CGROUP_NET_CLASSID)
>> 587 + nla_total_size(sizeof(u_int32_t)) /* classid */
>> 588 #endif
>> 589 + nla_total_size(sizeof(u_int32_t)); /* cap_len */
>> 590
>> 591 tstamp = skb_tstamp_cond(entskb, false);
>> 592 if (tstamp)
>> 593 size += nla_total_size(sizeof(struct nfqnl_msg_packet_timestamp));
>> 594
>> 595 size += nfqnl_get_bridge_size(entry);
>> 596
>> 597 if (entry->state.hook <= NF_INET_FORWARD ||
>> 598 (entry->state.hook == NF_INET_POST_ROUTING && entskb->sk == NULL))
>> 599 csum_verify = !skb_csum_unnecessary(entskb);
>> 600 else
>> 601 csum_verify = false;
>> 602
>> 603 outdev = entry->state.out;
>> 604
>> 605 switch ((enum nfqnl_config_mode)READ_ONCE(queue->copy_mode)) {
>> 606 case NFQNL_COPY_META:
>> 607 case NFQNL_COPY_NONE:
>> 608 break;
>> 609
>> 610 case NFQNL_COPY_PACKET:
>> 611 if (!(queue->flags & NFQA_CFG_F_GSO) &&
>> 612 entskb->ip_summed == CHECKSUM_PARTIAL &&
>> 613 nf_queue_checksum_help(entskb))
>> 614 return NULL;
>> 615
>> 616 data_len = READ_ONCE(queue->copy_range);
>> 617 if (data_len > entskb->len)
>> 618 data_len = entskb->len;
>> 619
>> 620 hlen = skb_zerocopy_headlen(entskb);
>> 621 hlen = min_t(unsigned int, hlen, data_len);
>> 622 size += sizeof(struct nlattr) + hlen;
>> 623 cap_len = entskb->len;
>> 624 break;
>> 625 }
>> 626
>> 627 nfnl_ct = rcu_dereference(nfnl_ct_hook);
>> 628
>> 629 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> 630 if (queue->flags & NFQA_CFG_F_CONNTRACK) {
>> 631 if (nfnl_ct != NULL) {
>> 632 ct = nf_ct_get(entskb, &ctinfo);
>> 633 if (ct != NULL)
>> 634 size += nfnl_ct->build_size(ct);
>> 635 }
>> 636 }
>> 637 #endif
>> 638
>> 639 if (queue->flags & NFQA_CFG_F_UID_GID) {
>> 640 size += (nla_total_size(sizeof(u_int32_t)) /* uid */
>> 641 + nla_total_size(sizeof(u_int32_t))); /* gid */
>> 642 }
>> 643
>> 644 if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
>> 645 seclen = nfqnl_get_sk_secctx(entskb, &ctx);
>>
>> nfqnl_get_sk_secctx() returns negative error codes. It needs to be changed ot
>> int as well instead of u32.
>>
>> 646 if (seclen >= 0)
>> 647 size += nla_total_size(seclen);
>> 648 }
>> 649
>> 650 skb = alloc_skb(size, GFP_ATOMIC);
>> 651 if (!skb) {
>> 652 skb_tx_error(entskb);
>> 653 goto nlmsg_failure;
>> 654 }
>> 655
>> 656 nlh = nfnl_msg_put(skb, 0, 0,
>> 657 nfnl_msg_type(NFNL_SUBSYS_QUEUE, NFQNL_MSG_PACKET),
>> 658 0, entry->state.pf, NFNETLINK_V0,
>> 659 htons(queue->queue_num));
>> 660 if (!nlh) {
>> 661 skb_tx_error(entskb);
>> 662 kfree_skb(skb);
>> 663 goto nlmsg_failure;
>> 664 }
>> 665
>> 666 nla = __nla_reserve(skb, NFQA_PACKET_HDR, sizeof(*pmsg));
>> 667 pmsg = nla_data(nla);
>> 668 pmsg->hw_protocol = entskb->protocol;
>> 669 pmsg->hook = entry->state.hook;
>> 670 *packet_id_ptr = &pmsg->packet_id;
>> 671
>> 672 indev = entry->state.in;
>> 673 if (indev) {
>> 674 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>> 675 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV, htonl(indev->ifindex)))
>> 676 goto nla_put_failure;
>> 677 #else
>> 678 if (entry->state.pf == PF_BRIDGE) {
>> 679 /* Case 1: indev is physical input device, we need to
>> 680 * look for bridge group (when called from
>> 681 * netfilter_bridge) */
>> 682 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
>> 683 htonl(indev->ifindex)) ||
>> 684 /* this is the bridge group "brX" */
>> 685 /* rcu_read_lock()ed by __nf_queue */
>> 686 nla_put_be32(skb, NFQA_IFINDEX_INDEV,
>> 687 htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
>> 688 goto nla_put_failure;
>> 689 } else {
>> 690 int physinif;
>> 691
>> 692 /* Case 2: indev is bridge group, we need to look for
>> 693 * physical device (when called from ipv4) */
>> 694 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV,
>> 695 htonl(indev->ifindex)))
>> 696 goto nla_put_failure;
>> 697
>> 698 physinif = nf_bridge_get_physinif(entskb);
>> 699 if (physinif &&
>> 700 nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
>> 701 htonl(physinif)))
>> 702 goto nla_put_failure;
>> 703 }
>> 704 #endif
>> 705 }
>> 706
>> 707 if (outdev) {
>> 708 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>> 709 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV, htonl(outdev->ifindex)))
>> 710 goto nla_put_failure;
>> 711 #else
>> 712 if (entry->state.pf == PF_BRIDGE) {
>> 713 /* Case 1: outdev is physical output device, we need to
>> 714 * look for bridge group (when called from
>> 715 * netfilter_bridge) */
>> 716 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
>> 717 htonl(outdev->ifindex)) ||
>> 718 /* this is the bridge group "brX" */
>> 719 /* rcu_read_lock()ed by __nf_queue */
>> 720 nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
>> 721 htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
>> 722 goto nla_put_failure;
>> 723 } else {
>> 724 int physoutif;
>> 725
>> 726 /* Case 2: outdev is bridge group, we need to look for
>> 727 * physical output device (when called from ipv4) */
>> 728 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
>> 729 htonl(outdev->ifindex)))
>> 730 goto nla_put_failure;
>> 731
>> 732 physoutif = nf_bridge_get_physoutif(entskb);
>> 733 if (physoutif &&
>> 734 nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
>> 735 htonl(physoutif)))
>> 736 goto nla_put_failure;
>> 737 }
>> 738 #endif
>> 739 }
>> 740
>> 741 if (entskb->mark &&
>> 742 nla_put_be32(skb, NFQA_MARK, htonl(entskb->mark)))
>> 743 goto nla_put_failure;
>> 744
>> 745 if (entskb->priority &&
>> 746 nla_put_be32(skb, NFQA_PRIORITY, htonl(entskb->priority)))
>> 747 goto nla_put_failure;
>> 748
>> 749 if (indev && entskb->dev &&
>> 750 skb_mac_header_was_set(entskb) &&
>> 751 skb_mac_header_len(entskb) != 0) {
>> 752 struct nfqnl_msg_packet_hw phw;
>> 753 int len;
>> 754
>> 755 memset(&phw, 0, sizeof(phw));
>> 756 len = dev_parse_header(entskb, phw.hw_addr);
>> 757 if (len) {
>> 758 phw.hw_addrlen = htons(len);
>> 759 if (nla_put(skb, NFQA_HWADDR, sizeof(phw), &phw))
>> 760 goto nla_put_failure;
>> 761 }
>> 762 }
>> 763
>> 764 if (nfqnl_put_bridge(entry, skb) < 0)
>> 765 goto nla_put_failure;
>> 766
>> 767 if (entry->state.hook <= NF_INET_FORWARD && tstamp) {
>> 768 struct nfqnl_msg_packet_timestamp ts;
>> 769 struct timespec64 kts = ktime_to_timespec64(tstamp);
>> 770
>> 771 ts.sec = cpu_to_be64(kts.tv_sec);
>> 772 ts.usec = cpu_to_be64(kts.tv_nsec / NSEC_PER_USEC);
>> 773
>> 774 if (nla_put(skb, NFQA_TIMESTAMP, sizeof(ts), &ts))
>> 775 goto nla_put_failure;
>> 776 }
>> 777
>> 778 if ((queue->flags & NFQA_CFG_F_UID_GID) && entskb->sk &&
>> 779 nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
>> 780 goto nla_put_failure;
>> 781
>> 782 if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
>> 783 goto nla_put_failure;
>> 784
>> 785 if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
>> ^^^^^^
>> This doesn't look right.
>>
>>
>> 786 goto nla_put_failure;
>> 787
>> 788 if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
>> 789 goto nla_put_failure;
>> 790
>> 791 if (cap_len > data_len &&
>> 792 nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
>> 793 goto nla_put_failure;
>> 794
>> 795 if (nfqnl_put_packet_info(skb, entskb, csum_verify))
>> 796 goto nla_put_failure;
>> 797
>> 798 if (data_len) {
>> 799 struct nlattr *nla;
>> 800
>> 801 if (skb_tailroom(skb) < sizeof(*nla) + hlen)
>> 802 goto nla_put_failure;
>> 803
>> 804 nla = skb_put(skb, sizeof(*nla));
>> 805 nla->nla_type = NFQA_PAYLOAD;
>> 806 nla->nla_len = nla_attr_size(data_len);
>> 807
>> 808 if (skb_zerocopy(skb, entskb, data_len, hlen))
>> 809 goto nla_put_failure;
>> 810 }
>> 811
>> 812 nlh->nlmsg_len = skb->len;
>> 813 if (seclen >= 0)
>> ^^^^^^^^^^^
>>
>> 814 security_release_secctx(&ctx);
>> 815 return skb;
>> 816
>> 817 nla_put_failure:
>> 818 skb_tx_error(entskb);
>> 819 kfree_skb(skb);
>> 820 net_err_ratelimited("nf_queue: error creating packet message\n");
>> 821 nlmsg_failure:
>> 822 if (seclen >= 0)
>> ^^^^^^^^^^^
>>
>> 823 security_release_secctx(&ctx);
>> 824 return NULL;
>> 825 }
>>
>>
>>
>> regards,
>> dan carpenter
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] lsm: replace context+len with lsm_context
2024-11-05 20:09 ` Casey Schaufler
@ 2024-11-05 21:29 ` Paul Moore
2024-11-06 10:10 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Paul Moore @ 2024-11-05 21:29 UTC (permalink / raw)
To: Casey Schaufler
Cc: Pablo Neira Ayuso, Dan Carpenter, netfilter-devel, LSM List
On Tue, Nov 5, 2024 at 3:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 11/5/2024 3:12 AM, Pablo Neira Ayuso wrote:
> > Hi,
> >
> > There is another issue in this recent series, see below.
> >
> > This needs another follow up.
>
> Dan Carpenter sent a patch:
>
> https://lore.kernel.org/lkml/b226a01a-2545-4b67-9cc6-59cfa0ffabbc@schaufler-ca.com/
... and I've been slacking so far this week. I'm reviewing and
merging these fixes now.
--
paul-moore.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] lsm: replace context+len with lsm_context
2024-11-05 20:09 ` Casey Schaufler
2024-11-05 21:29 ` Paul Moore
@ 2024-11-06 10:10 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2024-11-06 10:10 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Pablo Neira Ayuso, netfilter-devel, Paul Moore, LSM List
On Tue, Nov 05, 2024 at 12:09:49PM -0800, Casey Schaufler wrote:
> On 11/5/2024 3:12 AM, Pablo Neira Ayuso wrote:
> > Hi,
> >
> > There is another issue in this recent series, see below.
> >
> > This needs another follow up.
>
> Dan Carpenter sent a patch:
>
> https://lore.kernel.org/lkml/b226a01a-2545-4b67-9cc6-59cfa0ffabbc@schaufler-ca.com/
>
Sorry for the confusion. That was for a separate simpler issue. I was
confused about this line here.
785 if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
regards,
dan carpenter
> >
> > Thanks.
> >
> > On Sat, Nov 02, 2024 at 12:21:01PM +0300, Dan Carpenter wrote:
> >> Hello Casey Schaufler,
> >>
> >> Commit 95a3c11eb670 ("lsm: replace context+len with lsm_context")
> >> from Oct 23, 2024 (linux-next), leads to the following Smatch static
> >> checker warning:
> >>
> >> net/netfilter/nfnetlink_queue.c:646 nfqnl_build_packet_message()
> >> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
> >>
> >> net/netfilter/nfnetlink_queue.c:813 nfqnl_build_packet_message()
> >> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
> >>
> >> net/netfilter/nfnetlink_queue.c:822 nfqnl_build_packet_message()
> >> warn: always true condition '(seclen >= 0) => (0-u32max >= 0)'
> >>
> >> net/netfilter/nfnetlink_queue.c
> >> 551 static struct sk_buff *
> >> 552 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> >> 553 struct nf_queue_entry *entry,
> >> 554 __be32 **packet_id_ptr)
> >> 555 {
> >> 556 size_t size;
> >> 557 size_t data_len = 0, cap_len = 0;
> >> 558 unsigned int hlen = 0;
> >> 559 struct sk_buff *skb;
> >> 560 struct nlattr *nla;
> >> 561 struct nfqnl_msg_packet_hdr *pmsg;
> >> 562 struct nlmsghdr *nlh;
> >> 563 struct sk_buff *entskb = entry->skb;
> >> 564 struct net_device *indev;
> >> 565 struct net_device *outdev;
> >> 566 struct nf_conn *ct = NULL;
> >> 567 enum ip_conntrack_info ctinfo = 0;
> >> 568 const struct nfnl_ct_hook *nfnl_ct;
> >> 569 bool csum_verify;
> >> 570 struct lsm_context ctx;
> >> 571 u32 seclen = 0;
> >> ^^^^^^^^^^
> >>
> >> 572 ktime_t tstamp;
> >> 573
> >> 574 size = nlmsg_total_size(sizeof(struct nfgenmsg))
> >> 575 + nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
> >> 576 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> >> 577 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> >> 578 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> >> 579 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> >> 580 + nla_total_size(sizeof(u_int32_t)) /* ifindex */
> >> 581 #endif
> >> 582 + nla_total_size(sizeof(u_int32_t)) /* mark */
> >> 583 + nla_total_size(sizeof(u_int32_t)) /* priority */
> >> 584 + nla_total_size(sizeof(struct nfqnl_msg_packet_hw))
> >> 585 + nla_total_size(sizeof(u_int32_t)) /* skbinfo */
> >> 586 #if IS_ENABLED(CONFIG_CGROUP_NET_CLASSID)
> >> 587 + nla_total_size(sizeof(u_int32_t)) /* classid */
> >> 588 #endif
> >> 589 + nla_total_size(sizeof(u_int32_t)); /* cap_len */
> >> 590
> >> 591 tstamp = skb_tstamp_cond(entskb, false);
> >> 592 if (tstamp)
> >> 593 size += nla_total_size(sizeof(struct nfqnl_msg_packet_timestamp));
> >> 594
> >> 595 size += nfqnl_get_bridge_size(entry);
> >> 596
> >> 597 if (entry->state.hook <= NF_INET_FORWARD ||
> >> 598 (entry->state.hook == NF_INET_POST_ROUTING && entskb->sk == NULL))
> >> 599 csum_verify = !skb_csum_unnecessary(entskb);
> >> 600 else
> >> 601 csum_verify = false;
> >> 602
> >> 603 outdev = entry->state.out;
> >> 604
> >> 605 switch ((enum nfqnl_config_mode)READ_ONCE(queue->copy_mode)) {
> >> 606 case NFQNL_COPY_META:
> >> 607 case NFQNL_COPY_NONE:
> >> 608 break;
> >> 609
> >> 610 case NFQNL_COPY_PACKET:
> >> 611 if (!(queue->flags & NFQA_CFG_F_GSO) &&
> >> 612 entskb->ip_summed == CHECKSUM_PARTIAL &&
> >> 613 nf_queue_checksum_help(entskb))
> >> 614 return NULL;
> >> 615
> >> 616 data_len = READ_ONCE(queue->copy_range);
> >> 617 if (data_len > entskb->len)
> >> 618 data_len = entskb->len;
> >> 619
> >> 620 hlen = skb_zerocopy_headlen(entskb);
> >> 621 hlen = min_t(unsigned int, hlen, data_len);
> >> 622 size += sizeof(struct nlattr) + hlen;
> >> 623 cap_len = entskb->len;
> >> 624 break;
> >> 625 }
> >> 626
> >> 627 nfnl_ct = rcu_dereference(nfnl_ct_hook);
> >> 628
> >> 629 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> >> 630 if (queue->flags & NFQA_CFG_F_CONNTRACK) {
> >> 631 if (nfnl_ct != NULL) {
> >> 632 ct = nf_ct_get(entskb, &ctinfo);
> >> 633 if (ct != NULL)
> >> 634 size += nfnl_ct->build_size(ct);
> >> 635 }
> >> 636 }
> >> 637 #endif
> >> 638
> >> 639 if (queue->flags & NFQA_CFG_F_UID_GID) {
> >> 640 size += (nla_total_size(sizeof(u_int32_t)) /* uid */
> >> 641 + nla_total_size(sizeof(u_int32_t))); /* gid */
> >> 642 }
> >> 643
> >> 644 if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> >> 645 seclen = nfqnl_get_sk_secctx(entskb, &ctx);
> >>
> >> nfqnl_get_sk_secctx() returns negative error codes. It needs to be changed ot
> >> int as well instead of u32.
> >>
> >> 646 if (seclen >= 0)
> >> 647 size += nla_total_size(seclen);
> >> 648 }
> >> 649
> >> 650 skb = alloc_skb(size, GFP_ATOMIC);
> >> 651 if (!skb) {
> >> 652 skb_tx_error(entskb);
> >> 653 goto nlmsg_failure;
> >> 654 }
> >> 655
> >> 656 nlh = nfnl_msg_put(skb, 0, 0,
> >> 657 nfnl_msg_type(NFNL_SUBSYS_QUEUE, NFQNL_MSG_PACKET),
> >> 658 0, entry->state.pf, NFNETLINK_V0,
> >> 659 htons(queue->queue_num));
> >> 660 if (!nlh) {
> >> 661 skb_tx_error(entskb);
> >> 662 kfree_skb(skb);
> >> 663 goto nlmsg_failure;
> >> 664 }
> >> 665
> >> 666 nla = __nla_reserve(skb, NFQA_PACKET_HDR, sizeof(*pmsg));
> >> 667 pmsg = nla_data(nla);
> >> 668 pmsg->hw_protocol = entskb->protocol;
> >> 669 pmsg->hook = entry->state.hook;
> >> 670 *packet_id_ptr = &pmsg->packet_id;
> >> 671
> >> 672 indev = entry->state.in;
> >> 673 if (indev) {
> >> 674 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> >> 675 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV, htonl(indev->ifindex)))
> >> 676 goto nla_put_failure;
> >> 677 #else
> >> 678 if (entry->state.pf == PF_BRIDGE) {
> >> 679 /* Case 1: indev is physical input device, we need to
> >> 680 * look for bridge group (when called from
> >> 681 * netfilter_bridge) */
> >> 682 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
> >> 683 htonl(indev->ifindex)) ||
> >> 684 /* this is the bridge group "brX" */
> >> 685 /* rcu_read_lock()ed by __nf_queue */
> >> 686 nla_put_be32(skb, NFQA_IFINDEX_INDEV,
> >> 687 htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
> >> 688 goto nla_put_failure;
> >> 689 } else {
> >> 690 int physinif;
> >> 691
> >> 692 /* Case 2: indev is bridge group, we need to look for
> >> 693 * physical device (when called from ipv4) */
> >> 694 if (nla_put_be32(skb, NFQA_IFINDEX_INDEV,
> >> 695 htonl(indev->ifindex)))
> >> 696 goto nla_put_failure;
> >> 697
> >> 698 physinif = nf_bridge_get_physinif(entskb);
> >> 699 if (physinif &&
> >> 700 nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
> >> 701 htonl(physinif)))
> >> 702 goto nla_put_failure;
> >> 703 }
> >> 704 #endif
> >> 705 }
> >> 706
> >> 707 if (outdev) {
> >> 708 #if !IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> >> 709 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV, htonl(outdev->ifindex)))
> >> 710 goto nla_put_failure;
> >> 711 #else
> >> 712 if (entry->state.pf == PF_BRIDGE) {
> >> 713 /* Case 1: outdev is physical output device, we need to
> >> 714 * look for bridge group (when called from
> >> 715 * netfilter_bridge) */
> >> 716 if (nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
> >> 717 htonl(outdev->ifindex)) ||
> >> 718 /* this is the bridge group "brX" */
> >> 719 /* rcu_read_lock()ed by __nf_queue */
> >> 720 nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
> >> 721 htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
> >> 722 goto nla_put_failure;
> >> 723 } else {
> >> 724 int physoutif;
> >> 725
> >> 726 /* Case 2: outdev is bridge group, we need to look for
> >> 727 * physical output device (when called from ipv4) */
> >> 728 if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
> >> 729 htonl(outdev->ifindex)))
> >> 730 goto nla_put_failure;
> >> 731
> >> 732 physoutif = nf_bridge_get_physoutif(entskb);
> >> 733 if (physoutif &&
> >> 734 nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
> >> 735 htonl(physoutif)))
> >> 736 goto nla_put_failure;
> >> 737 }
> >> 738 #endif
> >> 739 }
> >> 740
> >> 741 if (entskb->mark &&
> >> 742 nla_put_be32(skb, NFQA_MARK, htonl(entskb->mark)))
> >> 743 goto nla_put_failure;
> >> 744
> >> 745 if (entskb->priority &&
> >> 746 nla_put_be32(skb, NFQA_PRIORITY, htonl(entskb->priority)))
> >> 747 goto nla_put_failure;
> >> 748
> >> 749 if (indev && entskb->dev &&
> >> 750 skb_mac_header_was_set(entskb) &&
> >> 751 skb_mac_header_len(entskb) != 0) {
> >> 752 struct nfqnl_msg_packet_hw phw;
> >> 753 int len;
> >> 754
> >> 755 memset(&phw, 0, sizeof(phw));
> >> 756 len = dev_parse_header(entskb, phw.hw_addr);
> >> 757 if (len) {
> >> 758 phw.hw_addrlen = htons(len);
> >> 759 if (nla_put(skb, NFQA_HWADDR, sizeof(phw), &phw))
> >> 760 goto nla_put_failure;
> >> 761 }
> >> 762 }
> >> 763
> >> 764 if (nfqnl_put_bridge(entry, skb) < 0)
> >> 765 goto nla_put_failure;
> >> 766
> >> 767 if (entry->state.hook <= NF_INET_FORWARD && tstamp) {
> >> 768 struct nfqnl_msg_packet_timestamp ts;
> >> 769 struct timespec64 kts = ktime_to_timespec64(tstamp);
> >> 770
> >> 771 ts.sec = cpu_to_be64(kts.tv_sec);
> >> 772 ts.usec = cpu_to_be64(kts.tv_nsec / NSEC_PER_USEC);
> >> 773
> >> 774 if (nla_put(skb, NFQA_TIMESTAMP, sizeof(ts), &ts))
> >> 775 goto nla_put_failure;
> >> 776 }
> >> 777
> >> 778 if ((queue->flags & NFQA_CFG_F_UID_GID) && entskb->sk &&
> >> 779 nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
> >> 780 goto nla_put_failure;
> >> 781
> >> 782 if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
> >> 783 goto nla_put_failure;
> >> 784
> >> 785 if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
> >> ^^^^^^
> >> This doesn't look right.
> >>
> >>
> >> 786 goto nla_put_failure;
> >> 787
> >> 788 if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> >> 789 goto nla_put_failure;
> >> 790
> >> 791 if (cap_len > data_len &&
> >> 792 nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
> >> 793 goto nla_put_failure;
> >> 794
> >> 795 if (nfqnl_put_packet_info(skb, entskb, csum_verify))
> >> 796 goto nla_put_failure;
> >> 797
> >> 798 if (data_len) {
> >> 799 struct nlattr *nla;
> >> 800
> >> 801 if (skb_tailroom(skb) < sizeof(*nla) + hlen)
> >> 802 goto nla_put_failure;
> >> 803
> >> 804 nla = skb_put(skb, sizeof(*nla));
> >> 805 nla->nla_type = NFQA_PAYLOAD;
> >> 806 nla->nla_len = nla_attr_size(data_len);
> >> 807
> >> 808 if (skb_zerocopy(skb, entskb, data_len, hlen))
> >> 809 goto nla_put_failure;
> >> 810 }
> >> 811
> >> 812 nlh->nlmsg_len = skb->len;
> >> 813 if (seclen >= 0)
> >> ^^^^^^^^^^^
> >>
> >> 814 security_release_secctx(&ctx);
> >> 815 return skb;
> >> 816
> >> 817 nla_put_failure:
> >> 818 skb_tx_error(entskb);
> >> 819 kfree_skb(skb);
> >> 820 net_err_ratelimited("nf_queue: error creating packet message\n");
> >> 821 nlmsg_failure:
> >> 822 if (seclen >= 0)
> >> ^^^^^^^^^^^
> >>
> >> 823 security_release_secctx(&ctx);
> >> 824 return NULL;
> >> 825 }
> >>
> >>
> >>
> >> regards,
> >> dan carpenter
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-11-06 10:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <549fd50d-a631-4103-bfe2-e842de387163@stanley.mountain>
2024-11-05 11:12 ` [bug report] lsm: replace context+len with lsm_context Pablo Neira Ayuso
2024-11-05 20:09 ` Casey Schaufler
2024-11-05 21:29 ` Paul Moore
2024-11-06 10:10 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox