[PATCH] acpi/erst: fix fallthrough code upon validation failure

Ani Sinha posted 1 patch 1 week ago
hw/acpi/erst.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] acpi/erst: fix fallthrough code upon validation failure
Posted by Ani Sinha 1 week ago
At any step when any validation fail in check_erst_backend_storage(), there is
no need to continue further through other validation checks. Further, by
continuing even when record_size is 0, we run the risk of triggering a divide
by zero error if we continued with other validation checks. Hence, we should
simply return from this function upon validation failure.

CC: Peter Maydell <peter.maydell@linaro.org>
CC: Eric DeVolder <eric.devolder@oracle.com>
Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/erst.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
index de509c2b48..df856b2669 100644
--- a/hw/acpi/erst.c
+++ b/hw/acpi/erst.c
@@ -440,6 +440,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
         (record_size >= 4096) /* PAGE_SIZE */
         )) {
         error_setg(errp, "ERST record_size %u is invalid", record_size);
+        return;
     }
 
     /* Validity check header */
@@ -450,6 +451,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
         (le16_to_cpu(header->reserved) == 0)
         )) {
         error_setg(errp, "ERST backend storage header is invalid");
+        return;
     }
 
     /* Check storage_size against record_size */
@@ -457,6 +459,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
          (record_size > s->storage_size)) {
         error_setg(errp, "ACPI ERST requires storage size be multiple of "
             "record size (%uKiB)", record_size);
+        return;
     }
 
     /* Compute offset of first and last record storage slot */
-- 
2.25.1
Re: [PATCH] acpi/erst: fix fallthrough code upon validation failure
Posted by Eric DeVolder 1 day, 4 hours ago

On 5/13/22 09:10, Ani Sinha wrote:
> At any step when any validation fail in check_erst_backend_storage(), there is
> no need to continue further through other validation checks. Further, by
> continuing even when record_size is 0, we run the risk of triggering a divide
> by zero error if we continued with other validation checks. Hence, we should
> simply return from this function upon validation failure.
> 
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Eric DeVolder <eric.devolder@oracle.com>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

Reviewed-by: Eric DeVolder <eric.devolder@oracle.com>

My apologies, I've been away for the past week.
Thank you for taking the time to correct this issue!
eric

> ---
>   hw/acpi/erst.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index de509c2b48..df856b2669 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -440,6 +440,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>           (record_size >= 4096) /* PAGE_SIZE */
>           )) {
>           error_setg(errp, "ERST record_size %u is invalid", record_size);
> +        return;
>       }
>   
>       /* Validity check header */
> @@ -450,6 +451,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>           (le16_to_cpu(header->reserved) == 0)
>           )) {
>           error_setg(errp, "ERST backend storage header is invalid");
> +        return;
>       }
>   
>       /* Check storage_size against record_size */
> @@ -457,6 +459,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>            (record_size > s->storage_size)) {
>           error_setg(errp, "ACPI ERST requires storage size be multiple of "
>               "record size (%uKiB)", record_size);
> +        return;
>       }
>   
>       /* Compute offset of first and last record storage slot */
>
Re: [PATCH] acpi/erst: fix fallthrough code upon validation failure
Posted by Igor Mammedov 1 day, 5 hours ago
On Fri, 13 May 2022 19:40:05 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> At any step when any validation fail in check_erst_backend_storage(), there is
> no need to continue further through other validation checks. Further, by
> continuing even when record_size is 0, we run the risk of triggering a divide
> by zero error if we continued with other validation checks. Hence, we should
> simply return from this function upon validation failure.
> 
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Eric DeVolder <eric.devolder@oracle.com>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/erst.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index de509c2b48..df856b2669 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -440,6 +440,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>          (record_size >= 4096) /* PAGE_SIZE */
>          )) {
>          error_setg(errp, "ERST record_size %u is invalid", record_size);
> +        return;
>      }
>  
>      /* Validity check header */
> @@ -450,6 +451,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>          (le16_to_cpu(header->reserved) == 0)
>          )) {
>          error_setg(errp, "ERST backend storage header is invalid");
> +        return;
>      }
>  
>      /* Check storage_size against record_size */
> @@ -457,6 +459,7 @@ static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
>           (record_size > s->storage_size)) {
>          error_setg(errp, "ACPI ERST requires storage size be multiple of "
>              "record size (%uKiB)", record_size);
> +        return;
>      }
>  
>      /* Compute offset of first and last record storage slot */