Re: [PATCH v4 06/17] sev: Fix error handling in sev_encrypt_flash()

[Daniel P . Berrangé]

On Wed, Jul 03, 2024 at 12:05:44PM +0100, Roy Hopkins wrote:
> The function sev_encrypt_flash() checks to see if the return value of
> launch_update_data() < 0, but the function returns a non-zero (and not
> necessarily negative) result on error. This means that some errors in
> updating launch data will result in the function returning success.

I'm not sure that positive return values are intended as an error ?
The sev_launch_update_data method does:

    if (!addr || !len) {
        return 1;
    }

which seems to suggest that passing a NULL addr / zero-length,
was intended to be treated as a no-op, rather than an error.

If that interpretation is wrong, then, I'd suggest that
sev_launch_update_data be changed to return '-1' in this
case, since QEMU standard is to consider negative values
as errors.

> 
> In addition, the function takes an Error parameter which is not used
> when an error is actually returned.
> 
> The return value is now checked for non-zero to indicate an error and
> a suitable error message is logged.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  target/i386/sev.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 3ab8b3c28b..491ca5369e 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1542,12 +1542,9 @@ sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t 
> len, Error **errp)
>  
>      /* if SEV is in update state then encrypt the data else do nothing */
>      if (sev_check_state(sev_common, SEV_STATE_LAUNCH_UPDATE)) {
> -        int ret;
> -
> -        ret = klass->launch_update_data(sev_common, gpa, ptr, len);
> -        if (ret < 0) {
> -            error_setg(errp, "SEV: Failed to encrypt pflash rom");
> -            return ret;
> +        if (klass->launch_update_data(sev_common, gpa, ptr, len)) {
> +            error_setg(errp, "SEV: Failed to encrypt flash");
> +            return -1;

sev_launch_update_data() calls error_report with the real error
details, and here we file a information-less error message.

Can we add "Error **errp" to the 'launch_update_data' API contract,
and change the error_report() calls to error_setg(), so we have
the useful information propagated in the Error object.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|