[PATCH v2 3/4] lib: sbi: fwft: Fix error return value for feature > 32 bits

[Anup Patel]

On Fri, Jan 24, 2025 at 10:23 PM Clément Léger <cleger at rivosinc.com> wrote:
>
> Based on the spec, invalid feature should return SBI_ERR_INVALID_PARAM.
> This was not the case with the current implementation since the upper 32
> bits were not considered when checking the feature thus returning
> SBI_ERR_DENIED. Fix that by passing the raw unsigned long rather than
> the enum to be sure we are passing the full 64 bits and then check for
> upper bits to be cleared.
>
> Signed-off-by: Clément Léger <cleger at rivosinc.com>
> ---
>  include/sbi/sbi_fwft.h |  4 ++--
>  lib/sbi/sbi_fwft.c     | 10 +++++++---
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/sbi/sbi_fwft.h b/include/sbi/sbi_fwft.h
> index 2148820e..a9caa863 100644
> --- a/include/sbi/sbi_fwft.h
> +++ b/include/sbi/sbi_fwft.h
> @@ -14,9 +14,9 @@
>
>  struct sbi_scratch;
>
> -int sbi_fwft_set(enum sbi_fwft_feature_t feature, unsigned long value,
> +int sbi_fwft_set(unsigned long feature, unsigned long value,
>                  unsigned long flags);
> -int sbi_fwft_get(enum sbi_fwft_feature_t feature, unsigned long *out_val);
> +int sbi_fwft_get(unsigned long feature, unsigned long *out_val);

The feature should be uint32_t as defined by the SBI spec.

Also, the uint32_t overflow check if required should be done in
sbi_ecall_fwft_handler().

Regards,
Anup

>
>  int sbi_fwft_init(struct sbi_scratch *scratch, bool cold_boot);
>
> diff --git a/lib/sbi/sbi_fwft.c b/lib/sbi/sbi_fwft.c
> index c46006d4..85f8a638 100644
> --- a/lib/sbi/sbi_fwft.c
> +++ b/lib/sbi/sbi_fwft.c
> @@ -287,12 +287,16 @@ static struct fwft_config* get_feature_config(enum sbi_fwft_feature_t feature)
>         return NULL;
>  }
>
> -static int fwft_get_feature(enum sbi_fwft_feature_t feature,
> +static int fwft_get_feature(unsigned long feature,
>                             struct fwft_config **conf)
>  {
>         int ret;
>         struct fwft_config *tconf;
>
> +       /* Feature are defined as 32 bits identifiers */
> +       if (feature & ~(BIT_ULL(32) - 1))
> +               return SBI_EINVAL;
> +
>         tconf = get_feature_config(feature);
>         if (!tconf) {
>                 if (fwft_is_defined_feature(feature))
> @@ -323,7 +327,7 @@ static void fwft_clear_config_lock(enum sbi_fwft_feature_t feature)
>         conf->flags &= ~SBI_FWFT_SET_FLAG_LOCK;
>  }
>
> -int sbi_fwft_set(enum sbi_fwft_feature_t feature, unsigned long value,
> +int sbi_fwft_set(unsigned long feature, unsigned long value,
>                  unsigned long flags)
>  {
>         int ret;
> @@ -348,7 +352,7 @@ int sbi_fwft_set(enum sbi_fwft_feature_t feature, unsigned long value,
>         return SBI_OK;
>  }
>
> -int sbi_fwft_get(enum sbi_fwft_feature_t feature, unsigned long *out_val)
> +int sbi_fwft_get(unsigned long feature, unsigned long *out_val)
>  {
>         int ret;
>         struct fwft_config *conf;
> --
> 2.47.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi