diff mbox series

[net-next] seg6: fix parameter passing when calling NF_HOOK() in End.DX4 and End.DX6 behaviors

Message ID 2a78f16a-0ff5-46bf-983b-9ab038f5a5cd@163.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] seg6: fix parameter passing when calling NF_HOOK() in End.DX4 and End.DX6 behaviors | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 902 this patch: 902
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: kuba@kernel.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 906 this patch: 906
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes fail Problems with Fixes tag: 1
netdev/build_allmodconfig_warn success Errors and warnings before: 906 this patch: 906
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-30--09-00 (tests: 1041)

Commit Message

Jianguo Wu May 30, 2024, 7:43 a.m. UTC 
From: Jianguo Wu <wujianguo@chinatelecom.cn>

input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
otherwise may trigger a NULL pointer dereference, as below:

    [74830.647293] BUG: kernel NULL pointer dereference, address: 0000000000000090
    [74830.655633] #PF: supervisor read access in kernel mode
    [74830.657888] #PF: error_code(0x0000) - not-present page
    [74830.659500] PGD 0 P4D 0
    [74830.660450] Oops: 0000 [#1] PREEMPT SMP PTI
    ...
    [74830.664953] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
    [74830.666569] RIP: 0010:rpfilter_mt+0x44/0x15e [ipt_rpfilter]
    ...
    [74830.689725] Call Trace:
    [74830.690402]  <IRQ>
    [74830.690953]  ? show_trace_log_lvl+0x1c4/0x2df
    [74830.692020]  ? show_trace_log_lvl+0x1c4/0x2df
    [74830.693095]  ? ipt_do_table+0x286/0x710 [ip_tables]
    [74830.694275]  ? __die_body.cold+0x8/0xd
    [74830.695205]  ? page_fault_oops+0xac/0x140
    [74830.696244]  ? exc_page_fault+0x62/0x150
    [74830.697225]  ? asm_exc_page_fault+0x22/0x30
    [74830.698344]  ? rpfilter_mt+0x44/0x15e [ipt_rpfilter]
    [74830.699540]  ipt_do_table+0x286/0x710 [ip_tables]
    [74830.700758]  ? ip6_route_input+0x19d/0x240
    [74830.701752]  nf_hook_slow+0x3f/0xb0
    [74830.702678]  input_action_end_dx4+0x19b/0x1e0
    [74830.703735]  ? input_action_end_t+0xe0/0xe0
    [74830.704734]  seg6_local_input_core+0x2d/0x60
    [74830.705782]  lwtunnel_input+0x5b/0xb0
    [74830.706690]  __netif_receive_skb_one_core+0x63/0xa0
    [74830.707825]  process_backlog+0x99/0x140
    [74830.709538]  __napi_poll+0x2c/0x160
    [74830.710673]  net_rx_action+0x296/0x350
    [74830.711860]  __do_softirq+0xcb/0x2ac
    [74830.713049]  do_softirq+0x63/0x90

input_action_end_dx4() passing a NULL indev to NF_HOOK(), and finally trigger a
NULL dereference in rpfilter_mt()->rpfilter_is_loopback():
    static bool
    rpfilter_is_loopback(const struct sk_buff *skb, const struct net_device *in)
    {
            // in is NULL
            return skb->pkt_type == PACKET_LOOPBACK || in->flags & IFF_LOOPBACK;
    }

Fixes: 7a3f5b0de364 ("netfilter: add netfilter hooks to SRv6 data plane")

Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 net/ipv6/seg6_local.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Simon Horman June 3, 2024, 6:21 p.m. UTC | #1 
On Thu, May 30, 2024 at 03:43:38PM +0800, Jianguo Wu wrote:
> From: Jianguo Wu <wujianguo@chinatelecom.cn>
> 
> input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
> for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
> otherwise may trigger a NULL pointer dereference, as below:

nit: The text above should be line-wrapped so that it is
     no more than 75 columns wide.

Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

> 
>     [74830.647293] BUG: kernel NULL pointer dereference, address: 0000000000000090
>     [74830.655633] #PF: supervisor read access in kernel mode
>     [74830.657888] #PF: error_code(0x0000) - not-present page
>     [74830.659500] PGD 0 P4D 0
>     [74830.660450] Oops: 0000 [#1] PREEMPT SMP PTI
>     ...
>     [74830.664953] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>     [74830.666569] RIP: 0010:rpfilter_mt+0x44/0x15e [ipt_rpfilter]
>     ...
>     [74830.689725] Call Trace:
>     [74830.690402]  <IRQ>
>     [74830.690953]  ? show_trace_log_lvl+0x1c4/0x2df
>     [74830.692020]  ? show_trace_log_lvl+0x1c4/0x2df
>     [74830.693095]  ? ipt_do_table+0x286/0x710 [ip_tables]
>     [74830.694275]  ? __die_body.cold+0x8/0xd
>     [74830.695205]  ? page_fault_oops+0xac/0x140
>     [74830.696244]  ? exc_page_fault+0x62/0x150
>     [74830.697225]  ? asm_exc_page_fault+0x22/0x30
>     [74830.698344]  ? rpfilter_mt+0x44/0x15e [ipt_rpfilter]
>     [74830.699540]  ipt_do_table+0x286/0x710 [ip_tables]
>     [74830.700758]  ? ip6_route_input+0x19d/0x240
>     [74830.701752]  nf_hook_slow+0x3f/0xb0
>     [74830.702678]  input_action_end_dx4+0x19b/0x1e0
>     [74830.703735]  ? input_action_end_t+0xe0/0xe0
>     [74830.704734]  seg6_local_input_core+0x2d/0x60
>     [74830.705782]  lwtunnel_input+0x5b/0xb0
>     [74830.706690]  __netif_receive_skb_one_core+0x63/0xa0
>     [74830.707825]  process_backlog+0x99/0x140
>     [74830.709538]  __napi_poll+0x2c/0x160
>     [74830.710673]  net_rx_action+0x296/0x350
>     [74830.711860]  __do_softirq+0xcb/0x2ac
>     [74830.713049]  do_softirq+0x63/0x90
> 
> input_action_end_dx4() passing a NULL indev to NF_HOOK(), and finally trigger a
> NULL dereference in rpfilter_mt()->rpfilter_is_loopback():
>     static bool
>     rpfilter_is_loopback(const struct sk_buff *skb, const struct net_device *in)
>     {
>             // in is NULL
>             return skb->pkt_type == PACKET_LOOPBACK || in->flags & IFF_LOOPBACK;
>     }
> 
> Fixes: 7a3f5b0de364 ("netfilter: add netfilter hooks to SRv6 data plane")

nit: no blank line here.

> 
> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>

I am slightly puzzled that this bug was in
the tree for so long without being noticed.

But the above not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...
Pablo Neira Ayuso June 3, 2024, 7:23 p.m. UTC | #2 
Hi,

On Thu, May 30, 2024 at 03:43:38PM +0800, Jianguo Wu wrote:
> From: Jianguo Wu <wujianguo@chinatelecom.cn>
> 
> input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
> for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
> otherwise may trigger a NULL pointer dereference, as below:

Could you also add a selftest to improve coverage of this infrastructure?

Thanks.
Jianguo Wu June 4, 2024, 6:33 a.m. UTC | #3 
Hi Simon,
  Thanks for your review, I will fix the commit log, and send v2 with your Reviewed-by tag.

On 2024/6/4 2:21, Simon Horman wrote:
> On Thu, May 30, 2024 at 03:43:38PM +0800, Jianguo Wu wrote:
>> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>>
>> input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
>> for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
>> otherwise may trigger a NULL pointer dereference, as below:
> 
> nit: The text above should be line-wrapped so that it is
>      no more than 75 columns wide.
> 
> Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
>>
>>     [74830.647293] BUG: kernel NULL pointer dereference, address: 0000000000000090
>>     [74830.655633] #PF: supervisor read access in kernel mode
>>     [74830.657888] #PF: error_code(0x0000) - not-present page
>>     [74830.659500] PGD 0 P4D 0
>>     [74830.660450] Oops: 0000 [#1] PREEMPT SMP PTI
>>     ...
>>     [74830.664953] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>>     [74830.666569] RIP: 0010:rpfilter_mt+0x44/0x15e [ipt_rpfilter]
>>     ...
>>     [74830.689725] Call Trace:
>>     [74830.690402]  <IRQ>
>>     [74830.690953]  ? show_trace_log_lvl+0x1c4/0x2df
>>     [74830.692020]  ? show_trace_log_lvl+0x1c4/0x2df
>>     [74830.693095]  ? ipt_do_table+0x286/0x710 [ip_tables]
>>     [74830.694275]  ? __die_body.cold+0x8/0xd
>>     [74830.695205]  ? page_fault_oops+0xac/0x140
>>     [74830.696244]  ? exc_page_fault+0x62/0x150
>>     [74830.697225]  ? asm_exc_page_fault+0x22/0x30
>>     [74830.698344]  ? rpfilter_mt+0x44/0x15e [ipt_rpfilter]
>>     [74830.699540]  ipt_do_table+0x286/0x710 [ip_tables]
>>     [74830.700758]  ? ip6_route_input+0x19d/0x240
>>     [74830.701752]  nf_hook_slow+0x3f/0xb0
>>     [74830.702678]  input_action_end_dx4+0x19b/0x1e0
>>     [74830.703735]  ? input_action_end_t+0xe0/0xe0
>>     [74830.704734]  seg6_local_input_core+0x2d/0x60
>>     [74830.705782]  lwtunnel_input+0x5b/0xb0
>>     [74830.706690]  __netif_receive_skb_one_core+0x63/0xa0
>>     [74830.707825]  process_backlog+0x99/0x140
>>     [74830.709538]  __napi_poll+0x2c/0x160
>>     [74830.710673]  net_rx_action+0x296/0x350
>>     [74830.711860]  __do_softirq+0xcb/0x2ac
>>     [74830.713049]  do_softirq+0x63/0x90
>>
>> input_action_end_dx4() passing a NULL indev to NF_HOOK(), and finally trigger a
>> NULL dereference in rpfilter_mt()->rpfilter_is_loopback():
>>     static bool
>>     rpfilter_is_loopback(const struct sk_buff *skb, const struct net_device *in)
>>     {
>>             // in is NULL
>>             return skb->pkt_type == PACKET_LOOPBACK || in->flags & IFF_LOOPBACK;
>>     }
>>
>> Fixes: 7a3f5b0de364 ("netfilter: add netfilter hooks to SRv6 data plane")
> 
> nit: no blank line here.
> 
>>
>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> 
> I am slightly puzzled that this bug was in
> the tree for so long without being noticed.
> 
> But the above not withstanding, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> ...
Jianguo Wu June 4, 2024, 6:34 a.m. UTC | #4 
Hi, Pablo

On 2024/6/4 3:23, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Thu, May 30, 2024 at 03:43:38PM +0800, Jianguo Wu wrote:
>> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>>
>> input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
>> for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
>> otherwise may trigger a NULL pointer dereference, as below:
> 
> Could you also add a selftest to improve coverage of this infrastructure?
> 

Sure, I will add a selftest to cover this case.

> Thanks.
Paolo Abeni June 4, 2024, 9:14 a.m. UTC | #5 
On Tue, 2024-06-04 at 14:34 +0800, Jianguo Wu wrote:
> Hi, Pablo
> 
> On 2024/6/4 3:23, Pablo Neira Ayuso wrote:
> > Hi,
> > 
> > On Thu, May 30, 2024 at 03:43:38PM +0800, Jianguo Wu wrote:
> > > From: Jianguo Wu <wujianguo@chinatelecom.cn>
> > > 
> > > input_action_end_dx4() and input_action_end_dx6() call NF_HOOK() for PREROUTING hook,
> > > for PREROUTING hook, we should passing a valid indev, and a NULL outdev to NF_HOOK(),
> > > otherwise may trigger a NULL pointer dereference, as below:
> > 
> > Could you also add a selftest to improve coverage of this infrastructure?
> > 
> 
> Sure, I will add a selftest to cover this case.

When re-submitting, please additionally target the 'net' tree,
replacing the 'net-next' prefix with 'net',

Thanks!

Paolo
diff mbox series

Patch

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 24e2b4b..c434940 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -941,8 +941,8 @@  static int input_action_end_dx6(struct sk_buff *skb,

 	if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled))
 		return NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
-			       dev_net(skb->dev), NULL, skb, NULL,
-			       skb_dst(skb)->dev, input_action_end_dx6_finish);
+			       dev_net(skb->dev), NULL, skb, skb->dev,
+			       NULL, input_action_end_dx6_finish);

 	return input_action_end_dx6_finish(dev_net(skb->dev), NULL, skb);
 drop:
@@ -991,8 +991,8 @@  static int input_action_end_dx4(struct sk_buff *skb,

 	if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled))
 		return NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING,
-			       dev_net(skb->dev), NULL, skb, NULL,
-			       skb_dst(skb)->dev, input_action_end_dx4_finish);
+			       dev_net(skb->dev), NULL, skb, skb->dev,
+			       NULL, input_action_end_dx4_finish);

 	return input_action_end_dx4_finish(dev_net(skb->dev), NULL, skb);
 drop: