netdev - Re: [PATCH net 12/15] ibmvnic: fix NULL pointer dereference in reset_sub_crq

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 Nov 2020 15:44:20 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lijun Pan <ljp@...ux.ibm.com>
Cc:     netdev@...r.kernel.org, sukadev@...ux.ibm.com, drt@...ux.ibm.com
Subject: Re: [PATCH net 12/15] ibmvnic: fix NULL pointer dereference in
 reset_sub_crq_queues

On Fri, 20 Nov 2020 16:40:46 -0600 Lijun Pan wrote:
> adapter->tx_scrq and adapter->rx_scrq could be NULL if the previous reset
> did not complete after freeing sub crqs. Check for NULL before
> dereferencing them.

> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 47446e5f8ec5..a0dbd963a1ab 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2930,6 +2930,13 @@ static int reset_sub_crq_queues(struct ibmvnic_adapter *adapter)
>  {
>  	int i, rc;
>  
> +	if (!adapter->tx_scrq || !adapter->rx_scrq) {
> +		netdev_err(adapter->netdev,
> +			   "tx_scrq (%p) or rx_scrq (%p) does not exist\n",
> +			   adapter->tx_scrq, adapter->rx_scrq);

This is expected to happen for the condition you describe in the commit
message. Either prevent it from happening or silently ignore.

What's the impact to the user when this happens? Why would they want to
know that some pointer is NULL? Presumably there is already a message
printed when reset does not complete or such?

> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < adapter->req_tx_queues; i++) {
>  		netdev_dbg(adapter->netdev, "Re-setting tx_scrq[%d]\n", i);
>  		rc = reset_one_sub_crq_queue(adapter, adapter->tx_scrq[i]);