On Fri, 09 Jul 2010 12:37:36 +0800 Axel Lin <[email protected]> wrote:
> When acpi_evaluate_object() is passed ACPI_ALLOCATE_BUFFER, > the caller must kfree the returned buffer if AE_OK is returned. > > Call Trace: > wmab_execute > -> wmi_evaluate_method > -> acpi_evaluate_object > > Thus if callers of wmab_execute() pass ACPI_ALLOCATE_BUFFER, > the return buffer must be kfreed if wmab_execute return AE_OK. > > Signed-off-by: Axel Lin <[email protected]> > --- > drivers/platform/x86/acer-wmi.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c > index 1ea6c43..3f44446 100644 > --- a/drivers/platform/x86/acer-wmi.c > +++ b/drivers/platform/x86/acer-wmi.c > @@ -555,6 +555,7 @@ static acpi_status AMW0_find_mailled(void) > obj->buffer.length == sizeof(struct wmab_ret)) { > ret = *((struct wmab_ret *) obj->buffer.pointer); > } else { > + kfree(out.pointer); > return AE_ERROR; > } > > @@ -598,6 +599,7 @@ static acpi_status AMW0_set_capabilities(void) > obj->buffer.length == sizeof(struct wmab_ret)) { > ret = *((struct wmab_ret *) obj->buffer.pointer); > } else { > + kfree(out.pointer); > return AE_ERROR; > } > > @@ -607,15 +609,22 @@ static acpi_status AMW0_set_capabilities(void) > args.ebx = 2 << 8; > args.ebx |= ACER_AMW0_BLUETOOTH_MASK; > > + /* > + * It's ok to use existing buffer for next wmab_execute call. > + * But we need to kfree(out.pointer) if next wmab_execute fail. > + */ > status = wmab_execute(&args, &out); > - if (ACPI_FAILURE(status)) > + if (ACPI_FAILURE(status)) { > + kfree(out.pointer); > return status; > + } > > obj = (union acpi_object *) out.pointer; > if (obj && obj->type == ACPI_TYPE_BUFFER > && obj->buffer.length == sizeof(struct wmab_ret)) { > ret = *((struct wmab_ret *) obj->buffer.pointer); > } else { > + kfree(out.pointer); > return AE_ERROR; > } I think it's best to remove the multiple-return-points approach in favour of the "goto single return point which undoes things" approach. that way we reduce the risk of later introducing more leaks. Please review: From: Andrew Morton <[email protected]> avoid multiple return points remove unneeded cast remove unneeded initialisation of `status' Cc: Alan Jenkins <[email protected]> Cc: Axel Lin <[email protected]> Cc: Carlos Corbacho <[email protected]> Cc: Matthew Garrett <[email protected]> Cc: Thomas Renninger <[email protected]> Signed-off-by: Andrew Morton <[email protected]> --- drivers/platform/x86/acer-wmi.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff -puN drivers/platform/x86/acer-wmi.c~acer-wmi-fix-memory-leaks-in-wmab_execute-error-path-v2-fix drivers/platform/x86/acer-wmi.c --- a/drivers/platform/x86/acer-wmi.c~acer-wmi-fix-memory-leaks-in-wmab_execute-error-path-v2-fix +++ a/drivers/platform/x86/acer-wmi.c @@ -571,7 +571,7 @@ static acpi_status AMW0_set_capabilities { struct wmab_args args; struct wmab_ret ret; - acpi_status status = AE_OK; + acpi_status status; struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *obj; @@ -594,13 +594,13 @@ static acpi_status AMW0_set_capabilities if (ACPI_FAILURE(status)) return status; - obj = (union acpi_object *) out.pointer; + obj = out.pointer; if (obj && obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == sizeof(struct wmab_ret)) { ret = *((struct wmab_ret *) obj->buffer.pointer); } else { - kfree(out.pointer); - return AE_ERROR; + status = AE_ERROR; + goto out; } if (ret.eax & 0x1) @@ -614,25 +614,21 @@ static acpi_status AMW0_set_capabilities * But we need to kfree(out.pointer) if next wmab_execute fail. */ status = wmab_execute(&args, &out); - if (ACPI_FAILURE(status)) { - kfree(out.pointer); - return status; - } + if (ACPI_FAILURE(status)) + goto out; obj = (union acpi_object *) out.pointer; if (obj && obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == sizeof(struct wmab_ret)) { ret = *((struct wmab_ret *) obj->buffer.pointer); } else { - kfree(out.pointer); - return AE_ERROR; + status = AE_ERROR; + goto out; } if (ret.eax & 0x1) interface->capability |= ACER_CAP_BLUETOOTH; - kfree(out.pointer); - /* * This appears to be safe to enable, since all Wistron based laptops * appear to use the same EC register for brightness, even if they @@ -641,7 +637,10 @@ static acpi_status AMW0_set_capabilities if (quirks->brightness >= 0) interface->capability |= ACER_CAP_BRIGHTNESS; - return AE_OK; + status = AE_OK; +out: + kfree(out.pointer); + return status; } static struct wmi_interface AMW0_interface = { _ -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
