ice: Don't dereference NULL in ice_gns_read error path

[PATCH net] ice: Don't dereference NULL in ice_gns_read error path

Posted by Simon Horman 2 years, 8 months ago

If pf is NULL in ice_gns_read() then it will be dereferenced
in the error path by a call to dev_dbg(ice_pf_to_dev(pf), ...).

Avoid this by simply returning in this case.
If logging is desired an alternate approach might be to
use pr_err() before returning.

Flagged by Smatch as:

  .../ice_gnss.c:196 ice_gnss_read() error: we previously assumed 'pf' could be null (see line 131)

Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 2ea8a2b11bcd..3d0663840aa1 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -128,12 +128,7 @@ static void ice_gnss_read(struct kthread_work *work)
 	int err = 0;
 
 	pf = gnss->back;
-	if (!pf) {
-		err = -EFAULT;
-		goto exit;
-	}
-
-	if (!test_bit(ICE_FLAG_GNSS, pf->flags))
+	if (!pf || !test_bit(ICE_FLAG_GNSS, pf->flags))
 		return;
 
 	hw = &pf->hw;
@@ -191,7 +186,6 @@ static void ice_gnss_read(struct kthread_work *work)
 	free_page((unsigned long)buf);
 requeue:
 	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
-exit:
 	if (err)
 		dev_dbg(ice_pf_to_dev(pf), "GNSS failed to read err=%d\n", err);
 }

Re: [PATCH net] ice: Don't dereference NULL in ice_gns_read error path

Posted by Tariq Toukan 2 years, 8 months ago


On 25/05/2023 13:52, Simon Horman wrote:
> If pf is NULL in ice_gns_read() then it will be dereferenced
> in the error path by a call to dev_dbg(ice_pf_to_dev(pf), ...).
> 
> Avoid this by simply returning in this case.
> If logging is desired an alternate approach might be to
> use pr_err() before returning.
> 
> Flagged by Smatch as:
> 
>    .../ice_gnss.c:196 ice_gnss_read() error: we previously assumed 'pf' could be null (see line 131)
> 
> Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> Signed-off-by: Simon Horman <horms@kernel.org>
> ---

LGTM.


Reviewed-by: Tariq Toukan <tariqt@nvidia.com>

RE: [Intel-wired-lan] [PATCH net] ice: Don't dereference NULL in ice_gns_read error path

Posted by Mekala, SunithaX D 2 years, 8 months ago

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Tariq Toukan
> Sent: Friday, May 26, 2023 2:48 AM
> To: Simon Horman <horms@kernel.org>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: Mishra, Sudhansu Sekhar <sudhansu.mishra@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; linux-kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>; Dan Carpenter <dan.carpenter@linaro.org>
> Subject: Re: [Intel-wired-lan] [PATCH net] ice: Don't dereference NULL in ice_gns_read error path
>
>
>
> On 25/05/2023 13:52, Simon Horman wrote:
>>  If pf is NULL in ice_gns_read() then it will be dereferenced in the 
>> error path by a call to dev_dbg(ice_pf_to_dev(pf), ...).
>>
>>  Avoid this by simply returning in this case.
>>  If logging is desired an alternate approach might be to use pr_err() 
>>  before returning.
>>  
>>  Flagged by Smatch as:
>>  
>>     .../ice_gnss.c:196 ice_gnss_read() error: we previously assumed 
>>  'pf' could be null (see line 131)
> >
>>  Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
>>  Signed-off-by: Simon Horman <horms@kernel.org>
>>  ---
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)