Re: [PATCH v1 iwl-net] igc: fix null pointer dereference in

[Kohei Enju <kohei@enjuk.jp>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3583 bytes --]

On Thu, 5 Feb 2026 12:16:06 +0200, "Lifshits, Vitaly" wrote:

> >> Hi Kohei,
> >>
> >> Thank you for your patch.
> >>
> >> Since there are no NVM-less devices I suggest removing the flash-less
> >> code entirely from the init flow.
> > 
> > Oh, I see there're no NVM-less devices. Then removing sounds good to me.
> > 
> > Could you clarify what you mean by "init flow"? Do you mean removing
> > only the flash-less branch in igc_init_nvm_params_i225(), or removing
> > all flash-less related code including igc_get_flash_presence_i225() and
> > its callers?
> > 
> > After clarification, I'd love to work on it. Thank you for taking a
> > look!
> 
> No, you shouldn’t remove this function.
> 
> However, if for any reason the flash is not present, the driver should 
> fail initialization.

I see. I understand we should fail igc_probe() for NVM-less devices.

> 
> There are two related places that need to be updated to enforce this:
> 
> igc_probe() in igc_main.c
> igc_init_nvm_params_i225() in igc_i225.c
> 
> This way we avoid supporting a configuration that doesn’t exist, and we 
> prevent the driver from partially initializing in an invalid state.

As far as I've skimmed the code, the only call trace is:

igc_probe()
  ei->get_invariants() (always igc_get_invariants_base())
    igc_init_nvm_params_i225()

so modifying igc_init_nvm_params_i225() is sufficient and IIUC we don't
have to modify igc_probe().

igc_init_nvm_params_i225() returns -EIO when there is no NVM, and its
caller igc_get_invariants_base() propagates the error back to
igc_probe().
Note that igc_get_invariants_base() currently ignores the return value
of igc_init_nvm_params_i225(), so I added that check as well.

diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
index 1613b562d17c..e4200279e15f 100644
--- a/drivers/net/ethernet/intel/igc/igc_base.c
+++ b/drivers/net/ethernet/intel/igc/igc_base.c
@@ -235,6 +235,9 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
                break;
        }

+       if (ret_val)
+               goto out;
+
        /* setup PHY parameters */
        ret_val = igc_init_phy_params_base(hw);
        if (ret_val)
diff --git a/drivers/net/ethernet/intel/igc/igc_i225.c b/drivers/net/ethernet/intel/igc/igc_i225.c
index 5226d10cc95b..ee1a8eeed9d5 100644
--- a/drivers/net/ethernet/intel/igc/igc_i225.c
+++ b/drivers/net/ethernet/intel/igc/igc_i225.c
@@ -476,21 +476,17 @@ s32 igc_init_nvm_params_i225(struct igc_hw *hw)
 {
        struct igc_nvm_info *nvm = &hw->nvm;

+       /* fail initialization for NVM-less devices */
+       if (!igc_get_flash_presence_i225(hw))
+               return -EIO;
+
        nvm->ops.acquire = igc_acquire_nvm_i225;
        nvm->ops.release = igc_release_nvm_i225;
+       nvm->ops.read = igc_read_nvm_srrd_i225;
+       nvm->ops.write = igc_write_nvm_srwr_i225;
+       nvm->ops.validate = igc_validate_nvm_checksum_i225;
+       nvm->ops.update = igc_update_nvm_checksum_i225;

-       /* NVM Function Pointers */
-       if (igc_get_flash_presence_i225(hw)) {
-               nvm->ops.read = igc_read_nvm_srrd_i225;
-               nvm->ops.write = igc_write_nvm_srwr_i225;
-               nvm->ops.validate = igc_validate_nvm_checksum_i225;
-               nvm->ops.update = igc_update_nvm_checksum_i225;
-       } else {
-               nvm->ops.read = igc_read_nvm_eerd;
-               nvm->ops.write = NULL;
-               nvm->ops.validate = NULL;
-               nvm->ops.update = NULL;
-       }
        return 0;
 }

Does this diff make sense to you?