| Message ID | 9930eadc36539f6135a0332116a51d48847505fe.1720004383.git.roy.hopkins@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce support for IGVM files | expand |
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
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; } }
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. 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(-)