| Message ID | 20220304183930.502777-1-tfanelli@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386/sev: Ensure attestation report length is valid before retrieving | expand |
On Fri, Mar 04, 2022 at 01:39:32PM -0500, Tyler Fanelli wrote: > The length of the attestation report buffer is never checked to be > valid before allocation is made. If the length of the report is returned > to be 0, the buffer to retrieve the attestation report is allocated with > length 0 and passed to the kernel to fill with contents of the attestation > report. Leaving this unchecked is dangerous and could lead to undefined > behavior. I don't see the undefined behaviour risk here. The KVM_SEV_ATTESTATION_REPORT semantics indicate that the kernel will fill in a valid length if the buffer we provide is too small and we can re-call with that buffer. If the kernel tells us the buffer is 0 bytes, then it should be fine having a second call with length 0. If not, then the kernel is broken and we're doomed. The QEMU code looks like it would cope with a zero length, unless I'm mistaken, it'll just return a zero length attestation report. > Signed-off-by: Tyler Fanelli <tfanelli@redhat.com> > --- > target/i386/sev.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 025ff7a6f8..215acd7c6b 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -616,6 +616,8 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce, > return NULL; > } > > + input.len = 0; The declaration of 'input' already zero initializes. > /* Query the report length */ > ret = sev_ioctl(sev->sev_fd, KVM_SEV_GET_ATTESTATION_REPORT, > &input, &err); > @@ -626,6 +628,11 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce, > ret, err, fw_error_to_str(err)); > return NULL; > } > + } else if (input.len <= 0) { It can't be less than 0 because 'len' is an unsigned integer. > + error_setg(errp, "SEV: Failed to query attestation report:" > + " length returned=%d", > + input.len); > + return NULL; > } Regards, Daniel
diff --git a/target/i386/sev.c b/target/i386/sev.c index 025ff7a6f8..215acd7c6b 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -616,6 +616,8 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce, return NULL; } + input.len = 0; + /* Query the report length */ ret = sev_ioctl(sev->sev_fd, KVM_SEV_GET_ATTESTATION_REPORT, &input, &err); @@ -626,6 +628,11 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce, ret, err, fw_error_to_str(err)); return NULL; } + } else if (input.len <= 0) { + error_setg(errp, "SEV: Failed to query attestation report:" + " length returned=%d", + input.len); + return NULL; } data = g_malloc(input.len);
The length of the attestation report buffer is never checked to be valid before allocation is made. If the length of the report is returned to be 0, the buffer to retrieve the attestation report is allocated with length 0 and passed to the kernel to fill with contents of the attestation report. Leaving this unchecked is dangerous and could lead to undefined behavior. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com> --- target/i386/sev.c | 7 +++++++ 1 file changed, 7 insertions(+)