Message ID | 20220304201141.509492-1-tfanelli@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v3] i386/sev: Ensure attestation report length is valid before retrieving | expand |
On 4/3/22 21:11, 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 buffer 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(+) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 025ff7a6f8..e82be3e350 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; I agree with Daniel's review of your v1: "The declaration of 'input' already zero initializes." https://lore.kernel.org/qemu-devel/YiJi9IYqtZvNQIRc@redhat.com/ > /* 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=%u", > + input.len); > + return NULL; > } > > data = g_malloc(input.len);
On Fri, Mar 04, 2022 at 03:11:43PM -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 buffer 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(+) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 025ff7a6f8..e82be3e350 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=%u", > + input.len); > + return NULL; I still feel the described scenario is a kernel bug, as QEMU handles len == 0 safely already AFAICT. I can't see how the upstream kernel would end up in the problem state. Can you show the buggy kernel code upstream. Regards, Daniel
diff --git a/target/i386/sev.c b/target/i386/sev.c index 025ff7a6f8..e82be3e350 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=%u", + 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 buffer 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(+)