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

[Kalle Valo <kvalo@codeaurora.org>]

Luca Coelho <luca@coelho.fi> writes:

> On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
>> Luca Coelho <luca@coelho.fi> writes:
>> 
>> > From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>> > 
>> > 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 <christophe.jaillet@wanadoo.fr>
>> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>> > ---
>> >  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 = &wifi_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