linux-kernel - Re: [PATCH] net/atm/mpc: Fix dereference NULL pointer in mpc_send

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Feb 2023 10:59:05 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Li Qiong <liqiong@...china.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Yu Zhe <yuzhe@...china.com>
Subject: Re: [PATCH] net/atm/mpc: Fix dereference NULL pointer in
 mpc_send_packet()

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@...china.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