[PATCH net] fix null-deref in ipv4_link_failure

[PATCH net] fix null-deref in ipv4_link_failure

[Kyle Zeng]

Currently, we assume the skb is associated with a device before calling
__ip_options_compile, which is not always the case if it is re-routed by
ipvs.
When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
This patch adds a check for the edge case and switch to use the net_device
from the rtable when skb->dev is NULL.

Suggested-by: Paolo Abeni<pabeni@redhat.com>
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 net/ipv4/route.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d8c99bdc617..1be34e5eea1 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1214,6 +1214,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
 static void ipv4_send_dest_unreach(struct sk_buff *skb)
 {
 	struct ip_options opt;
+	struct net_device *dev;
 	int res;
 
 	/* Recompile ip options since IPCB may not be valid anymore.
@@ -1230,7 +1231,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
 		opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
 
 		rcu_read_lock();
-		res = __ip_options_compile(dev_net(skb->dev), &opt, skb, NULL);
+		dev = skb->dev ? skb->dev : skb_rtable(skb)->dst.dev;
+		res = __ip_options_compile(dev_net(dev), &opt, skb, NULL);
 		rcu_read_unlock();
 
 		if (res)
-- 
2.34.1

Re: [PATCH net] fix null-deref in ipv4_link_failure

[Vadim Fedorenko]

On 08.09.2023 04:18, Kyle Zeng wrote:
> Currently, we assume the skb is associated with a device before calling
> __ip_options_compile, which is not always the case if it is re-routed by
> ipvs.
> When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> This patch adds a check for the edge case and switch to use the net_device
> from the rtable when skb->dev is NULL.
> 
> Suggested-by: Paolo Abeni<pabeni@redhat.com>
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
> Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
>   net/ipv4/route.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index d8c99bdc617..1be34e5eea1 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1214,6 +1214,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
>   static void ipv4_send_dest_unreach(struct sk_buff *skb)
>   {
>   	struct ip_options opt;
> +	struct net_device *dev;

Please, use reverse x-mas tree order for local variables

>   	int res;
>   
>   	/* Recompile ip options since IPCB may not be valid anymore.
> @@ -1230,7 +1231,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>   		opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
>   
>   		rcu_read_lock();
> -		res = __ip_options_compile(dev_net(skb->dev), &opt, skb, NULL);
> +		dev = skb->dev ? skb->dev : skb_rtable(skb)->dst.dev;
> +		res = __ip_options_compile(dev_net(dev), &opt, skb, NULL);
>   		rcu_read_unlock();
>   
>   		if (res)

Re: [PATCH net] fix null-deref in ipv4_link_failure

[David Ahern]

On 9/7/23 9:18 PM, Kyle Zeng wrote:
> Currently, we assume the skb is associated with a device before calling
> __ip_options_compile, which is not always the case if it is re-routed by
> ipvs.
> When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> This patch adds a check for the edge case and switch to use the net_device
> from the rtable when skb->dev is NULL.
> 
> Suggested-by: Paolo Abeni<pabeni@redhat.com>
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
> Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
>  net/ipv4/route.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net] fix null-deref in ipv4_link_failure

[Paolo Abeni]

On Thu, 2023-09-07 at 20:18 -0700, Kyle Zeng wrote:
> Currently, we assume the skb is associated with a device before calling
> __ip_options_compile, which is not always the case if it is re-routed by
> ipvs.
> When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> This patch adds a check for the edge case and switch to use the net_device
> from the rtable when skb->dev is NULL.
> 
> Suggested-by: Paolo Abeni<pabeni@redhat.com>
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
> Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
>  net/ipv4/route.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index d8c99bdc617..1be34e5eea1 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1214,6 +1214,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
>  static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  {
>  	struct ip_options opt;
> +	struct net_device *dev;
>  	int res;
>  
>  	/* Recompile ip options since IPCB may not be valid anymore.
> @@ -1230,7 +1231,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  		opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
>  
>  		rcu_read_lock();
> -		res = __ip_options_compile(dev_net(skb->dev), &opt, skb, NULL);
> +		dev = skb->dev ? skb->dev : skb_rtable(skb)->dst.dev;
> +		res = __ip_options_compile(dev_net(dev), &opt, skb, NULL);
>  		rcu_read_unlock();
>  
>  		if (res)

The patch LGTM, but if you are going to re-post to address Vadim's
feedback, please additionally drop my Suggested-by. I only reviewed a
previous revision.

Thanks!

Paolo

[PATCH net] fix null-deref in ipv4_link_failure

[Kyle Zeng]

Currently, we assume the skb is associated with a device before calling
__ip_options_compile, which is not always the case if it is re-routed by
ipvs.
When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
This patch adds a check for the edge case and switch to use the net_device
from the rtable when skb->dev is NULL.

Fixes: ed0de45 ("ipv4: recompile ip options in ipv4_link_failure")
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
Cc: Vadim Fedorenko <vfedorenko@novek.ru>
---
 net/ipv4/route.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d8c99bdc617..cba0d148c27 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1213,6 +1213,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
 
 static void ipv4_send_dest_unreach(struct sk_buff *skb)
 {
+	struct net_device *dev;
 	struct ip_options opt;
 	int res;
 
@@ -1230,7 +1231,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
 		opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
 
 		rcu_read_lock();
-		res = __ip_options_compile(dev_net(skb->dev), &opt, skb, NULL);
+		dev = skb->dev ? skb->dev : skb_rtable(skb)->dst.dev;
+		res = __ip_options_compile(dev_net(dev), &opt, skb, NULL);
 		rcu_read_unlock();
 
 		if (res)
-- 
2.34.1

Re: [PATCH net] fix null-deref in ipv4_link_failure

[David Ahern]

On 9/11/23 8:35 PM, Kyle Zeng wrote:
> Currently, we assume the skb is associated with a device before calling
> __ip_options_compile, which is not always the case if it is re-routed by
> ipvs.
> When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> This patch adds a check for the edge case and switch to use the net_device
> from the rtable when skb->dev is NULL.
> 
> Fixes: ed0de45 ("ipv4: recompile ip options in ipv4_link_failure")
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
> Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
> Cc: Vadim Fedorenko <vfedorenko@novek.ru>
> ---
>  net/ipv4/route.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net] fix null-deref in ipv4_link_failure

[Paolo Abeni]

On Mon, 2023-09-11 at 19:35 -0700, Kyle Zeng wrote:
> Currently, we assume the skb is associated with a device before calling
> __ip_options_compile, which is not always the case if it is re-routed by
> ipvs.
> When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> This patch adds a check for the edge case and switch to use the net_device
> from the rtable when skb->dev is NULL.
> 
> Fixes: ed0de45 ("ipv4: recompile ip options in ipv4_link_failure")

I'm sorry for the nit-picking, but please use 12 chars to identify the
blamed commit or we risk (future) conflicts.

Please also add a subsystem tag inside the subj. Finally include a
revision counter into prefix, alike:

[PATCH v3 net] ipv4: fix ...

(I'm not really sure about the correct counter here, since a few
versions flied without it)

Note that you can retain David's ack in the next revision.


Thanks!

Paolo