Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

2017-08-07 Thread Luca Coelho 
On Mon, 2017-08-07 at 16:37 +0300, Kalle Valo wrote:
> Luca Coelho  writes:
> 
> > On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
> > > Luca Coelho  writes:
> > > 
> > > > From: Christophe Jaillet 
> > > > 
> > > > We should free 'wgds.pointer' here as done a few lines above in another
> > > > error handling path.
> > > > It was allocated within 'acpi_evaluate_object()'.
> > > > 
> > > > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for 
> > > > geographic tx power table")
> > > > Signed-off-by: Christophe JAILLET 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 --
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c 
> > > > b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > index 79e7a7a285dc..82863e9273eb 100644
> > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct 
> > > > iwl_mvm *mvm)
> > > >  
> > > > entry = _pkg->package.elements[idx++];
> > > > if ((entry->type != ACPI_TYPE_INTEGER) ||
> > > > -   (entry->integer.value > U8_MAX))
> > > > -   return -EINVAL;
> > > > +   (entry->integer.value > U8_MAX)) {
> > > > +   ret = -EINVAL;
> > > > +   goto out_free;
> > > > +   }
> > > 
> > > How likely is this leak to happen in real world? To me it looks like
> > > more like a theoretical issue and could have easily waited for 4.14. But
> > > it's fine this time, just something to keep in mind in the future.
> > 
> > This is a one-liner fix and I consider memory leaks serious enough to
> > deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
> > trust me, broken ACPI tables are not that hard to find.
> 
> Sure, anything's possible. But what I'm reading here this is still a
> theoretical issue, not a leak which we _know_ will happen to thousands
> of people.
> 
> > But you rule here, feel free to NACK my patches whenever you see fit! :)
> 
> I'm trying to minimise the numbers of patches going to wireless-drivers
> and striving for only fixes which really matter and keep the theoretical
> stuff for -next. The is mostly selfish reasons as wireless-drivers are a
> lot more work, especially if there are conflicts.

I totally understand.  It's a lot more work for me too, with all the
reordering I need to do and the conflicts these cause.


> But like I said in my previous mail, no need to drop this.

Okay, thanks!

--
Cheers,
Luca.

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

2017-08-07 Thread Kalle Valo 
Luca Coelho  writes:

> On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
>> Luca Coelho  writes:
>> 
>> > From: Christophe Jaillet 
>> > 
>> > We should free 'wgds.pointer' here as done a few lines above in another
>> > error handling path.
>> > It was allocated within 'acpi_evaluate_object()'.
>> > 
>> > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for 
>> > geographic tx power table")
>> > Signed-off-by: Christophe JAILLET 
>> > Signed-off-by: Luca Coelho 
>> > ---
>> >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 --
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c 
>> > b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > index 79e7a7a285dc..82863e9273eb 100644
>> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct 
>> > iwl_mvm *mvm)
>> >  
>> >entry = _pkg->package.elements[idx++];
>> >if ((entry->type != ACPI_TYPE_INTEGER) ||
>> > -  (entry->integer.value > U8_MAX))
>> > -  return -EINVAL;
>> > +  (entry->integer.value > U8_MAX)) {
>> > +  ret = -EINVAL;
>> > +  goto out_free;
>> > +  }
>> 
>> How likely is this leak to happen in real world? To me it looks like
>> more like a theoretical issue and could have easily waited for 4.14. But
>> it's fine this time, just something to keep in mind in the future.
>
> This is a one-liner fix and I consider memory leaks serious enough to
> deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
> trust me, broken ACPI tables are not that hard to find.

Sure, anything's possible. But what I'm reading here this is still a
theoretical issue, not a leak which we _know_ will happen to thousands
of people.

> But you rule here, feel free to NACK my patches whenever you see fit! :)

I'm trying to minimise the numbers of patches going to wireless-drivers
and striving for only fixes which really matter and keep the theoretical
stuff for -next. The is mostly selfish reasons as wireless-drivers are a
lot more work, especially if there are conflicts.

But like I said in my previous mail, no need to drop this.

-- 
Kalle Valo

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

2017-08-07 Thread Luca Coelho 
On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
> Luca Coelho  writes:
> 
> > From: Christophe Jaillet 
> > 
> > We should free 'wgds.pointer' here as done a few lines above in another
> > error handling path.
> > It was allocated within 'acpi_evaluate_object()'.
> > 
> > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for 
> > geographic tx power table")
> > Signed-off-by: Christophe JAILLET 
> > Signed-off-by: Luca Coelho 
> > ---
> >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c 
> > b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > index 79e7a7a285dc..82863e9273eb 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm 
> > *mvm)
> >  
> > entry = _pkg->package.elements[idx++];
> > if ((entry->type != ACPI_TYPE_INTEGER) ||
> > -   (entry->integer.value > U8_MAX))
> > -   return -EINVAL;
> > +   (entry->integer.value > U8_MAX)) {
> > +   ret = -EINVAL;
> > +   goto out_free;
> > +   }
> 
> How likely is this leak to happen in real world? To me it looks like
> more like a theoretical issue and could have easily waited for 4.14. But
> it's fine this time, just something to keep in mind in the future.

This is a one-liner fix and I consider memory leaks serious enough to
deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
trust me, broken ACPI tables are not that hard to find.

But you rule here, feel free to NACK my patches whenever you see fit! :)

--
Cheers,
Luca.

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

2017-08-07 Thread Kalle Valo 
Luca Coelho  writes:

> From: Christophe Jaillet 
>
> We should free 'wgds.pointer' here as done a few lines above in another
> error handling path.
> It was allocated within 'acpi_evaluate_object()'.
>
> Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic 
> tx power table")
> Signed-off-by: Christophe JAILLET 
> Signed-off-by: Luca Coelho 
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c 
> b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> index 79e7a7a285dc..82863e9273eb 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm 
> *mvm)
>  
>   entry = _pkg->package.elements[idx++];
>   if ((entry->type != ACPI_TYPE_INTEGER) ||
> - (entry->integer.value > U8_MAX))
> - return -EINVAL;
> + (entry->integer.value > U8_MAX)) {
> + ret = -EINVAL;
> + goto out_free;
> + }

How likely is this leak to happen in real world? To me it looks like
more like a theoretical issue and could have easily waited for 4.14. But
it's fine this time, just something to keep in mind in the future.

-- 
Kalle Valo

[PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

2017-08-05 Thread Luca Coelho 
From: Christophe Jaillet 

We should free 'wgds.pointer' here as done a few lines above in another
error handling path.
It was allocated within 'acpi_evaluate_object()'.

Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic 
tx power table")
Signed-off-by: Christophe JAILLET 
Signed-off-by: Luca Coelho 
---
 drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c 
b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index 79e7a7a285dc..82863e9273eb 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm 
*mvm)
 
entry = _pkg->package.elements[idx++];
if ((entry->type != ACPI_TYPE_INTEGER) ||
-   (entry->integer.value > U8_MAX))
-   return -EINVAL;
+   (entry->integer.value > U8_MAX)) {
+   ret = -EINVAL;
+   goto out_free;
+   }
 
mvm->geo_profiles[i].values[j] = entry->integer.value;
}
-- 
2.13.2