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.hopk...@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 :|
