| Message ID | 20190330001712.8923-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
| Series | platform/x86: alienware-wmi: fix kfree on potentially uninitialized pointer | expand |
On Sat, Mar 30, 2019 at 12:17:12AM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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 <colin.king@canonical.com> Thanks for the catch Colin, queued for testing. Did you trigger this error or detect it via review or static analysis?
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 <colin.king@canonical.com> >> >> 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 <colin.king@canonical.com> > > 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 <colin.king@canonical.com> > >> > >> 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 <colin.king@canonical.com> > > > > 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.
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 <colin.king@canonical.com> >>>> >>>> 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 <colin.king@canonical.com> >>> >>> 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.
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; - } /*