[Intel-wired-lan] [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Intel-wired-lan] [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Yuan Can]

The pci_register_driver() can fail and when this happened, the dca_notifier
needs to be unregistered, otherwise the dca_notifier can be called when
igb fails to install, resulting to invalid memory access.

Fixes: fe4506b6a2f9 ("igb: add DCA support")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f1d088168723..18284a838e24 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -637,6 +637,10 @@ static int __init igb_init_module(void)
 	dca_register_notify(&dca_notifier);
 #endif
 	ret = pci_register_driver(&igb_driver);
+#ifdef CONFIG_IGB_DCA
+	if (ret)
+		dca_unregister_notify(&dca_notifier);
+#endif
 	return ret;
 }
 
-- 
2.17.1

[PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Yuan Can]

The pci_register_driver() can fail and when this happened, the dca_notifier
needs to be unregistered, otherwise the dca_notifier can be called when
igb fails to install, resulting to invalid memory access.

Fixes: fe4506b6a2f9 ("igb: add DCA support")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f1d088168723..18284a838e24 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -637,6 +637,10 @@ static int __init igb_init_module(void)
 	dca_register_notify(&dca_notifier);
 #endif
 	ret = pci_register_driver(&igb_driver);
+#ifdef CONFIG_IGB_DCA
+	if (ret)
+		dca_unregister_notify(&dca_notifier);
+#endif
 	return ret;
 }
 
-- 
2.17.1

Re: [Intel-wired-lan] [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Simon Horman]

+ Alexander Duyck

On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
> The pci_register_driver() can fail and when this happened, the dca_notifier
> needs to be unregistered, otherwise the dca_notifier can be called when
> igb fails to install, resulting to invalid memory access.
> 
> Fixes: fe4506b6a2f9 ("igb: add DCA support")

I don't think this problem was introduced by the commit cited above,
as it added the call to dca_unregister_notify() before
pci_register_driver(). But rather by the commit cited below which reversed
the order of these function calls.

bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")

I'm unsure if it is necessary to repost the patch to address that.
But if you do, and assuming we are treating this as a bug fix,
please target it for the net (or iwl-net) tree like this:

Subject: [PATCH net v2] ...

> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index f1d088168723..18284a838e24 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
>  	dca_register_notify(&dca_notifier);
>  #endif
>  	ret = pci_register_driver(&igb_driver);
> +#ifdef CONFIG_IGB_DCA
> +	if (ret)
> +		dca_unregister_notify(&dca_notifier);
> +#endif
>  	return ret;
>  }
>  
> -- 
> 2.17.1
> 
> 

Re: [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Simon Horman]

+ Alexander Duyck

On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
> The pci_register_driver() can fail and when this happened, the dca_notifier
> needs to be unregistered, otherwise the dca_notifier can be called when
> igb fails to install, resulting to invalid memory access.
> 
> Fixes: fe4506b6a2f9 ("igb: add DCA support")

I don't think this problem was introduced by the commit cited above,
as it added the call to dca_unregister_notify() before
pci_register_driver(). But rather by the commit cited below which reversed
the order of these function calls.

bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")

I'm unsure if it is necessary to repost the patch to address that.
But if you do, and assuming we are treating this as a bug fix,
please target it for the net (or iwl-net) tree like this:

Subject: [PATCH net v2] ...

> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index f1d088168723..18284a838e24 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
>  	dca_register_notify(&dca_notifier);
>  #endif
>  	ret = pci_register_driver(&igb_driver);
> +#ifdef CONFIG_IGB_DCA
> +	if (ret)
> +		dca_unregister_notify(&dca_notifier);
> +#endif
>  	return ret;
>  }
>  
> -- 
> 2.17.1
> 
> 

Re: [Intel-wired-lan] [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Alexander Duyck]

On Tue, Oct 22, 2024 at 8:56 AM Simon Horman <horms@kernel.org> wrote:
>
> + Alexander Duyck
>
> On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
> > The pci_register_driver() can fail and when this happened, the dca_notifier
> > needs to be unregistered, otherwise the dca_notifier can be called when
> > igb fails to install, resulting to invalid memory access.
> >
> > Fixes: fe4506b6a2f9 ("igb: add DCA support")
>
> I don't think this problem was introduced by the commit cited above,
> as it added the call to dca_unregister_notify() before
> pci_register_driver(). But rather by the commit cited below which reversed
> the order of these function calls.
>
> bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")
>
> I'm unsure if it is necessary to repost the patch to address that.
> But if you do, and assuming we are treating this as a bug fix,
> please target it for the net (or iwl-net) tree like this:
>
> Subject: [PATCH net v2] ...
>
> > Signed-off-by: Yuan Can <yuancan@huawei.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index f1d088168723..18284a838e24 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
> >       dca_register_notify(&dca_notifier);
> >  #endif
> >       ret = pci_register_driver(&igb_driver);
> > +#ifdef CONFIG_IGB_DCA
> > +     if (ret)
> > +             dca_unregister_notify(&dca_notifier);
> > +#endif
> >       return ret;
> >  }
> >

Makes sense to me. I agree on the "Fix DCA errors" patch being the one
that is being fixed. So essentially this is a notifier leak since we
are registering it but not unregistering.

Thanks,

- Alex

Re: [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Alexander Duyck]

On Tue, Oct 22, 2024 at 8:56 AM Simon Horman <horms@kernel.org> wrote:
>
> + Alexander Duyck
>
> On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
> > The pci_register_driver() can fail and when this happened, the dca_notifier
> > needs to be unregistered, otherwise the dca_notifier can be called when
> > igb fails to install, resulting to invalid memory access.
> >
> > Fixes: fe4506b6a2f9 ("igb: add DCA support")
>
> I don't think this problem was introduced by the commit cited above,
> as it added the call to dca_unregister_notify() before
> pci_register_driver(). But rather by the commit cited below which reversed
> the order of these function calls.
>
> bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")
>
> I'm unsure if it is necessary to repost the patch to address that.
> But if you do, and assuming we are treating this as a bug fix,
> please target it for the net (or iwl-net) tree like this:
>
> Subject: [PATCH net v2] ...
>
> > Signed-off-by: Yuan Can <yuancan@huawei.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index f1d088168723..18284a838e24 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
> >       dca_register_notify(&dca_notifier);
> >  #endif
> >       ret = pci_register_driver(&igb_driver);
> > +#ifdef CONFIG_IGB_DCA
> > +     if (ret)
> > +             dca_unregister_notify(&dca_notifier);
> > +#endif
> >       return ret;
> >  }
> >

Makes sense to me. I agree on the "Fix DCA errors" patch being the one
that is being fixed. So essentially this is a notifier leak since we
are registering it but not unregistering.

Thanks,

- Alex

Re: [Intel-wired-lan] [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Yuan Can]

On 2024/10/22 23:56, Simon Horman wrote:
> + Alexander Duyck
>
> On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
>> The pci_register_driver() can fail and when this happened, the dca_notifier
>> needs to be unregistered, otherwise the dca_notifier can be called when
>> igb fails to install, resulting to invalid memory access.
>>
>> Fixes: fe4506b6a2f9 ("igb: add DCA support")
> I don't think this problem was introduced by the commit cited above,
> as it added the call to dca_unregister_notify() before
> pci_register_driver(). But rather by the commit cited below which reversed
> the order of these function calls.
>
> bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")
>
> I'm unsure if it is necessary to repost the patch to address that.
> But if you do, and assuming we are treating this as a bug fix,
> please target it for the net (or iwl-net) tree like this:
>
> Subject: [PATCH net v2] ...
Ok, I will send a v2 patch to the net tree, thanks!
>> Signed-off-by: Yuan Can <yuancan@huawei.com>
>> ---
>>   drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index f1d088168723..18284a838e24 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
>>   	dca_register_notify(&dca_notifier);
>>   #endif
>>   	ret = pci_register_driver(&igb_driver);
>> +#ifdef CONFIG_IGB_DCA
>> +	if (ret)
>> +		dca_unregister_notify(&dca_notifier);
>> +#endif
>>   	return ret;
>>   }
>>   
>> -- 
>> 2.17.1
>>
>>
-- 
Best regards,
Yuan Can

Re: [PATCH] igb: Fix potential invalid memory access in igb_init_module()

[Yuan Can]

On 2024/10/22 23:56, Simon Horman wrote:
> + Alexander Duyck
>
> On Tue, Oct 22, 2024 at 02:38:07PM +0800, Yuan Can wrote:
>> The pci_register_driver() can fail and when this happened, the dca_notifier
>> needs to be unregistered, otherwise the dca_notifier can be called when
>> igb fails to install, resulting to invalid memory access.
>>
>> Fixes: fe4506b6a2f9 ("igb: add DCA support")
> I don't think this problem was introduced by the commit cited above,
> as it added the call to dca_unregister_notify() before
> pci_register_driver(). But rather by the commit cited below which reversed
> the order of these function calls.
>
> bbd98fe48a43 ("igb: Fix DCA errors and do not use context index for 82576")
>
> I'm unsure if it is necessary to repost the patch to address that.
> But if you do, and assuming we are treating this as a bug fix,
> please target it for the net (or iwl-net) tree like this:
>
> Subject: [PATCH net v2] ...
Ok, I will send a v2 patch to the net tree, thanks!
>> Signed-off-by: Yuan Can <yuancan@huawei.com>
>> ---
>>   drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index f1d088168723..18284a838e24 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -637,6 +637,10 @@ static int __init igb_init_module(void)
>>   	dca_register_notify(&dca_notifier);
>>   #endif
>>   	ret = pci_register_driver(&igb_driver);
>> +#ifdef CONFIG_IGB_DCA
>> +	if (ret)
>> +		dca_unregister_notify(&dca_notifier);
>> +#endif
>>   	return ret;
>>   }
>>   
>> -- 
>> 2.17.1
>>
>>
-- 
Best regards,
Yuan Can