From: Colin Ian King <[email protected]>
Currently the kfree of output.pointer can be potentially freeing
an uninitalized pointer in the case where out_data is NULL. Fix this
by reworking the case where out_data is not-null to perform the
ACPI status check and also the kfree of outpoint.pointer in one block
and hence ensuring the pointer is only freed when it has been used.
Also replace the if (ptr != NULL) idiom with just if (ptr).
Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/platform/x86/alienware-wmi.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index f10af5c383c5..c0d1555735cd 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -522,23 +522,22 @@ static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
input.length = (acpi_size) sizeof(*in_args);
input.pointer = in_args;
- if (out_data != NULL) {
+ if (out_data) {
output.length = ACPI_ALLOCATE_BUFFER;
output.pointer = NULL;
status = wmi_evaluate_method(WMAX_CONTROL_GUID, 0,
command, &input, &output);
- } else
+ if (ACPI_SUCCESS(status)) {
+ obj = (union acpi_object *)output.pointer;
+ if (obj && obj->type == ACPI_TYPE_INTEGER)
+ *out_data = (u32)obj->integer.value;
+ }
+ kfree(output.pointer);
+ } else {
status = wmi_evaluate_method(WMAX_CONTROL_GUID, 0,
command, &input, NULL);
-
- if (ACPI_SUCCESS(status) && out_data != NULL) {
- obj = (union acpi_object *)output.pointer;
- if (obj && obj->type == ACPI_TYPE_INTEGER)
- *out_data = (u32) obj->integer.value;
}
- kfree(output.pointer);
return status;
-
}
/*
--
2.20.1
On Sat, Mar 30, 2019 at 12:17:12AM +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> Currently the kfree of output.pointer can be potentially freeing
> an uninitalized pointer in the case where out_data is NULL. Fix this
> by reworking the case where out_data is not-null to perform the
> ACPI status check and also the kfree of outpoint.pointer in one block
> and hence ensuring the pointer is only freed when it has been used.
>
> Also replace the if (ptr != NULL) idiom with just if (ptr).
>
> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
> Signed-off-by: Colin Ian King <[email protected]>
Thanks for the catch Colin, queued for testing.
Did you trigger this error or detect it via review or static analysis?
--
Darren Hart
VMware Open Source Technology Center
On 03/04/2019 23:02, Darren Hart wrote:
> On Sat, Mar 30, 2019 at 12:17:12AM +0000, Colin King wrote:
>> From: Colin Ian King <[email protected]>
>>
>> Currently the kfree of output.pointer can be potentially freeing
>> an uninitalized pointer in the case where out_data is NULL. Fix this
>> by reworking the case where out_data is not-null to perform the
>> ACPI status check and also the kfree of outpoint.pointer in one block
>> and hence ensuring the pointer is only freed when it has been used.
>>
>> Also replace the if (ptr != NULL) idiom with just if (ptr).
>>
>> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
>> Signed-off-by: Colin Ian King <[email protected]>
>
> Thanks for the catch Colin, queued for testing.
>
> Did you trigger this error or detect it via review or static analysis?
>
Static analysis, I'm now running a licensed version of Coverity on one
of our servers.
Colin
On Wed, Apr 03, 2019 at 11:05:12PM +0100, Colin Ian King wrote:
> On 03/04/2019 23:02, Darren Hart wrote:
> > On Sat, Mar 30, 2019 at 12:17:12AM +0000, Colin King wrote:
> >> From: Colin Ian King <[email protected]>
> >>
> >> Currently the kfree of output.pointer can be potentially freeing
> >> an uninitalized pointer in the case where out_data is NULL. Fix this
> >> by reworking the case where out_data is not-null to perform the
> >> ACPI status check and also the kfree of outpoint.pointer in one block
> >> and hence ensuring the pointer is only freed when it has been used.
> >>
> >> Also replace the if (ptr != NULL) idiom with just if (ptr).
> >>
> >> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
> >> Signed-off-by: Colin Ian King <[email protected]>
> >
> > Thanks for the catch Colin, queued for testing.
> >
> > Did you trigger this error or detect it via review or static analysis?
> >
> Static analysis, I'm now running a licensed version of Coverity on one
> of our servers.
We typically include the tool used to identify such bugs, and I see several such
tags for Coverity in the logs. Was there a reason not to include that tag? If
just an oversight, can you provide that tag and I'll amend the commit.
--
Darren Hart
VMware Open Source Technology Center
On 03/04/2019 23:26, Darren Hart wrote:
> On Wed, Apr 03, 2019 at 11:05:12PM +0100, Colin Ian King wrote:
>> On 03/04/2019 23:02, Darren Hart wrote:
>>> On Sat, Mar 30, 2019 at 12:17:12AM +0000, Colin King wrote:
>>>> From: Colin Ian King <[email protected]>
>>>>
>>>> Currently the kfree of output.pointer can be potentially freeing
>>>> an uninitalized pointer in the case where out_data is NULL. Fix this
>>>> by reworking the case where out_data is not-null to perform the
>>>> ACPI status check and also the kfree of outpoint.pointer in one block
>>>> and hence ensuring the pointer is only freed when it has been used.
>>>>
>>>> Also replace the if (ptr != NULL) idiom with just if (ptr).
>>>>
>>>> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
>>>> Signed-off-by: Colin Ian King <[email protected]>
>>>
>>> Thanks for the catch Colin, queued for testing.
>>>
>>> Did you trigger this error or detect it via review or static analysis?
>>>
>> Static analysis, I'm now running a licensed version of Coverity on one
>> of our servers.
>
> We typically include the tool used to identify such bugs, and I see several such
> tags for Coverity in the logs. Was there a reason not to include that tag? If
> just an oversight, can you provide that tag and I'll amend the commit.
>
I didn't have an external coverity CID# number so I omitted it this time.