| Message ID | 20241029233232.27692-1-antonio@mandelbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | amdgpu: prevent NULL pointer dereference if ATIF is not supported | expand |
Hi Mario, On 30/10/2024 02:41, Mario Limonciello wrote: > On 10/29/2024 18:32, Antonio Quartulli wrote: >> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which >> would result in dereferencing buffer.pointer (obj) while being NULL. >> >> Bail out also when status is AE_NOT_FOUND with a proper error message. >> >> This fixes 1 FORWARD_NULL issue reported by Coverity >> Report: CID 1600951: Null pointer dereferences (FORWARD_NULL) >> >> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com> > > I'm not really sure how realistic this failure is. Can you share the > full call trace that Coverity identified? I just checked Coverity Scan and it only says: 5. Condition status, taking true branch. 6. Condition status != 5U /* (acpi_status)(5 | 0) */, taking false branch. The above points are related to: if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) It doesn't show how acpi_evaluate_object() is expected to return AE_NOT_FOUND. This said, if you think this case is unrealistic, why do you check for "status != AE_NOT_FOUND" at all? At this point maybe it would make more sense to simply drop this check and always bail out with the current error message. Basically a patch with the following only: - /* Fail if calling the method fails and ATIF is supported */ - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { + /* Fail if calling the method fails */ + if (ACPI_FAILURE(status)) { This way we don't make a fuzz for a possibly unrealistic case, while still protecting against bugs and null-dereferences. Regards, > > amdgpu_atif_pci_probe_handle() will check whether the handle is > available in the first place. We'll never this this far if that failed. > > Because of that I don't follow how this could return AE_NOT_FOUND. >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ >> drm/amd/amdgpu/amdgpu_acpi.c >> index cce85389427f..f10c3261a4ab 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >> @@ -172,10 +172,13 @@ static union acpi_object >> *amdgpu_atif_call(struct amdgpu_atif *atif, >> &buffer); >> obj = (union acpi_object *)buffer.pointer; >> - /* Fail if calling the method fails and ATIF is supported */ >> - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { >> - DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >> - acpi_format_exception(status)); >> + /* Fail if calling the method fails */ >> + if (ACPI_FAILURE(status)) { >> + if (status != AE_NOT_FOUND) >> + DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >> + acpi_format_exception(status)); >> + else >> + DRM_DEBUG_DRIVER("ATIF not supported\n"); >> kfree(obj); >> return NULL; >> } >
On 10/30/2024 16:06, Antonio Quartulli wrote: > Hi Mario, > > On 30/10/2024 02:41, Mario Limonciello wrote: >> On 10/29/2024 18:32, Antonio Quartulli wrote: >>> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which >>> would result in dereferencing buffer.pointer (obj) while being NULL. >>> >>> Bail out also when status is AE_NOT_FOUND with a proper error message. >>> >>> This fixes 1 FORWARD_NULL issue reported by Coverity >>> Report: CID 1600951: Null pointer dereferences (FORWARD_NULL) >>> >>> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com> >> >> I'm not really sure how realistic this failure is. Can you share the >> full call trace that Coverity identified? > > I just checked Coverity Scan and it only says: > > 5. Condition status, taking true branch. > 6. Condition status != 5U /* (acpi_status)(5 | 0) */, taking false > branch. > > The above points are related to: > > if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) > > It doesn't show how acpi_evaluate_object() is expected to return > AE_NOT_FOUND. > > This said, if you think this case is unrealistic, why do you check for > "status != AE_NOT_FOUND" at all? > > At this point maybe it would make more sense to simply drop this check > and always bail out with the current error message. > > Basically a patch with the following only: > > - /* Fail if calling the method fails and ATIF is supported */ > - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { > + /* Fail if calling the method fails */ > + if (ACPI_FAILURE(status)) { > > This way we don't make a fuzz for a possibly unrealistic case, while > still protecting against bugs and null-dereferences. Yeah I think that's a good idea. Can you respin it as a v2? > > > Regards, > >> >> amdgpu_atif_pci_probe_handle() will check whether the handle is >> available in the first place. We'll never this this far if that failed. >> >> Because of that I don't follow how this could return AE_NOT_FOUND. >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ >>> drm/amd/amdgpu/amdgpu_acpi.c >>> index cce85389427f..f10c3261a4ab 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >>> @@ -172,10 +172,13 @@ static union acpi_object >>> *amdgpu_atif_call(struct amdgpu_atif *atif, >>> &buffer); >>> obj = (union acpi_object *)buffer.pointer; >>> - /* Fail if calling the method fails and ATIF is supported */ >>> - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { >>> - DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >>> - acpi_format_exception(status)); >>> + /* Fail if calling the method fails */ >>> + if (ACPI_FAILURE(status)) { >>> + if (status != AE_NOT_FOUND) >>> + DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >>> + acpi_format_exception(status)); >>> + else >>> + DRM_DEBUG_DRIVER("ATIF not supported\n"); >>> kfree(obj); >>> return NULL; >>> } >> >
On 31/10/2024 15:41, Mario Limonciello wrote: > On 10/30/2024 16:06, Antonio Quartulli wrote: >> Hi Mario, >> >> On 30/10/2024 02:41, Mario Limonciello wrote: >>> On 10/29/2024 18:32, Antonio Quartulli wrote: >>>> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which >>>> would result in dereferencing buffer.pointer (obj) while being NULL. >>>> >>>> Bail out also when status is AE_NOT_FOUND with a proper error message. >>>> >>>> This fixes 1 FORWARD_NULL issue reported by Coverity >>>> Report: CID 1600951: Null pointer dereferences (FORWARD_NULL) >>>> >>>> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com> >>> >>> I'm not really sure how realistic this failure is. Can you share the >>> full call trace that Coverity identified? >> >> I just checked Coverity Scan and it only says: >> >> 5. Condition status, taking true branch. >> 6. Condition status != 5U /* (acpi_status)(5 | 0) */, taking >> false branch. >> >> The above points are related to: >> >> if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) >> >> It doesn't show how acpi_evaluate_object() is expected to return >> AE_NOT_FOUND. >> >> This said, if you think this case is unrealistic, why do you check for >> "status != AE_NOT_FOUND" at all? >> >> At this point maybe it would make more sense to simply drop this check >> and always bail out with the current error message. >> >> Basically a patch with the following only: >> >> - /* Fail if calling the method fails and ATIF is supported */ >> - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { >> + /* Fail if calling the method fails */ >> + if (ACPI_FAILURE(status)) { >> >> This way we don't make a fuzz for a possibly unrealistic case, while >> still protecting against bugs and null-dereferences. > > Yeah I think that's a good idea. Can you respin it as a v2? Will do! Thanks for your feedback, Mario. Regards, > >> >> >> Regards, >> >>> >>> amdgpu_atif_pci_probe_handle() will check whether the handle is >>> available in the first place. We'll never this this far if that failed. >>> >>> Because of that I don't follow how this could return AE_NOT_FOUND. >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++++++---- >>>> 1 file changed, 7 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ >>>> drm/amd/amdgpu/amdgpu_acpi.c >>>> index cce85389427f..f10c3261a4ab 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c >>>> @@ -172,10 +172,13 @@ static union acpi_object >>>> *amdgpu_atif_call(struct amdgpu_atif *atif, >>>> &buffer); >>>> obj = (union acpi_object *)buffer.pointer; >>>> - /* Fail if calling the method fails and ATIF is supported */ >>>> - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { >>>> - DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >>>> - acpi_format_exception(status)); >>>> + /* Fail if calling the method fails */ >>>> + if (ACPI_FAILURE(status)) { >>>> + if (status != AE_NOT_FOUND) >>>> + DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", >>>> + acpi_format_exception(status)); >>>> + else >>>> + DRM_DEBUG_DRIVER("ATIF not supported\n"); >>>> kfree(obj); >>>> return NULL; >>>> } >>> >> >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c index cce85389427f..f10c3261a4ab 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c @@ -172,10 +172,13 @@ static union acpi_object *amdgpu_atif_call(struct amdgpu_atif *atif, &buffer); obj = (union acpi_object *)buffer.pointer; - /* Fail if calling the method fails and ATIF is supported */ - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { - DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", - acpi_format_exception(status)); + /* Fail if calling the method fails */ + if (ACPI_FAILURE(status)) { + if (status != AE_NOT_FOUND) + DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", + acpi_format_exception(status)); + else + DRM_DEBUG_DRIVER("ATIF not supported\n"); kfree(obj); return NULL; }
acpi_evaluate_object() may return AE_NOT_FOUND (failure), which would result in dereferencing buffer.pointer (obj) while being NULL. Bail out also when status is AE_NOT_FOUND with a proper error message. This fixes 1 FORWARD_NULL issue reported by Coverity Report: CID 1600951: Null pointer dereferences (FORWARD_NULL) Signed-off-by: Antonio Quartulli <antonio@mandelbit.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)