[PATCH] net/atm/mpc: Fix dereference NULL pointer in mpc_send_packet()

Li Qiong posted 1 patch 3 years, 1 month ago
net/atm/mpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] net/atm/mpc: Fix dereference NULL pointer in mpc_send_packet()
Posted by Li Qiong 3 years, 1 month ago
The 'non_ip' statement need do 'mpc' pointer dereference,
so return '-ENODEV' if 'mpc' is NULL.

Signed-off-by: Li Qiong <liqiong@nfschina.com>
---
 net/atm/mpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index 033871e718a3..1cd6610b8a12 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -577,7 +577,7 @@ static netdev_tx_t mpc_send_packet(struct sk_buff *skb,
 	mpc = find_mpc_by_lec(dev); /* this should NEVER fail */
 	if (mpc == NULL) {
 		pr_info("(%s) no MPC found\n", dev->name);
-		goto non_ip;
+		return -ENODEV;
 	}
 
 	eth = (struct ethhdr *)skb->data;
-- 
2.11.0
Re: [PATCH] net/atm/mpc: Fix dereference NULL pointer in mpc_send_packet()
Posted by Dan Carpenter 3 years, 1 month ago
On Thu, Feb 23, 2023 at 02:54:46PM +0800, Li Qiong wrote:
> The 'non_ip' statement need do 'mpc' pointer dereference,
> so return '-ENODEV' if 'mpc' is NULL.
> 
> Signed-off-by: Li Qiong <liqiong@nfschina.com>
> ---
>  net/atm/mpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index 033871e718a3..1cd6610b8a12 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -577,7 +577,7 @@ static netdev_tx_t mpc_send_packet(struct sk_buff *skb,
>  	mpc = find_mpc_by_lec(dev); /* this should NEVER fail */
>  	if (mpc == NULL) {
>  		pr_info("(%s) no MPC found\n", dev->name);
> -		goto non_ip;
> +		return -ENODEV;
>  	}

The comment says that find_mpc_by_lec() can't fail.  This business of
handling impossible situations is very complicated.  Should this
free the skb before returning?  Eventually static checkers will detect
that.

Generally the rule is that we don't have checks for impossible
conditions.  That would trigger a warning for certain static checkers
but it would be a false positive.  Otherwise we need to add a whole
bunch of code to silence all warnings about handling an impossible
situation correctly.

regards,
dan carpenter