From: Dan Williams <dan.j.williams@intel.com>
To: Alexey Kardashevskiy <aik@amd.com>,
Dan Williams <dan.j.williams@intel.com>,
<linux-coco@lists.linux.dev>
Cc: Borislav Petkov <bp@alien8.de>,
Tom Lendacky <thomas.lendacky@amd.com>,
Dionna Glaze <dionnaglaze@google.com>,
Brijesh Singh <brijesh.singh@amd.com>,
Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>,
"Kuppuswamy Sathyanarayanan"
<sathyanarayanan.kuppuswamy@linux.intel.com>,
<peterz@infradead.org>, <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
Date: Wed, 18 Oct 2023 21:43:41 -0700 [thread overview]
Message-ID: <6530b3fda1e2d_5c0d29451@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <d52fedac-c1a1-43e4-abce-f6bca273f352@amd.com>
Alexey Kardashevskiy wrote:
[..]
> > Yes, it's been allowed in loop declaration for a few kernels now and the
> > new __free() helper in cleanup.h will make this model more prominent. My
> > sense is that it is still not open season on mid-function declarations,
> > but __attribute__((__cleanup__())) usage needs it.
> >
> >> Since you are doing this, move zero_ent below. Or, better, use
> >> guid_is_null().
> >
> > guid_is_null() is not techinically enough given the specification
> > mandates that the entire entry is zero.
>
> Well technically it says that guid, offset and length must be zeroes.
> So, (guid_is_null() && !offset && !length) and no yet another static
> declaration of a bunch of zeroes far away. I even wonder if there is a
> helper to check if some memory is all zeroes :) Up to you.
Ok, I switched to comparing each field to zero.
[..]
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + memcpy(&hdr, buf, sizeof(hdr));
> >>> + if (hdr.status == SNP_REPORT_INVALID_PARAM)
> >>> + return -EINVAL;
> >>> + if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> >>> + return -EINVAL;
> >>> + if (hdr.status)
> >>> + return -ENXIO;
> >>> + if ((hdr.report_size + sizeof(hdr)) > report_size)
> >>> + return -ENOMEM;
> >>> +
> >>> + void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> >>> + if (!rbuf)
> >>> + return -ENOMEM;
> >>> +
> >>> + memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> >>> + report->outblob = no_free_ptr(rbuf);
> >>> + report->outblob_len = hdr.report_size;
> >>> +
> >>> + certs_size = 0;
> >>> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> >>> + if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
> >>> + break;
> >>> + certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
> >>> + }
> >>> +
> >>> + /* No certs to report */
> >>> + if (!certs_size)
> >>
> >> Nit: WARN_ON_ONCE(i) here?
> >
> > Seems harsh for what could only be a firmware bug, panic_on_warn users
> > would not appreciate crashing the kernel over something recoverable like
> > this.
>
> This would a HV bug as certificates come from the KVM. And it is
> (slightly) more likely that the HV is trying to trigger buffer overrun
> in the guest.
Added a dev_warn_ratelimited(). WARN_ON_ONCE() is too heavy as I expect
panic_on_warn policy has more reason to be deployed in a confidential VM
than other places.
>
> >>
> >>> + return 0;
> >>> +
> >>> + /*
> >>> + * cert_table reports more data than fits in ext_size the
> >>> + * userspace cert_table walker can decide what happens next,
> >>> + * truncate the output
> >>> + */
> >>> + if (certs_size > ext_size)
> >>> + certs_size = ext_size;
> >>
> >> This sounds more like the HV provided a broken table with offset(s)
> >> ouside of the certs buffer. The HV is expected instead return
> >> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
> >> guest to retry.
> >
> > The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
> > prepared to retry. Retry support would be a follow-on new capability.
>
> My point is that you should not get into the situation when this
> calculated certs_size is greater than ext_size. If this is the case
> because someone sent too many certificaties via /dev/sev or kvmfd on the
> host, the GHCB call won't return any certs and will ask for a retry instead.
I understand, but given this needs to walk the entries anyway to size
the buffer correctly this sanity check is "free".
> >>> +
> >>> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> >>> + if (!cbuf)
> >>> + return -ENOMEM;
> >>
> >> In a such (unlikely) event the function returns an error but does not
> >> free report->outblob which is going to leak if consequent call succeded.
> >> This new no_free_ptr business is confusing at times :(
> >
> > If this fails it results in the attribute read failing and
> > read_generation does not advance. The next read attempt will free the
> > partially completed report and retry,
>
> Ah ok. In general, it just feels like every use of no_free_ptr() defeats
> the whole purpose of __free(xxx).
no_free_ptr() is there to say "we correctly populated this buffer,
there are no more error returns in this function, transition the
responsibility of freeing this buffer to the object it was assigned".
next prev parent reply other threads:[~2023-10-19 4:43 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-13 2:13 [PATCH v6 0/7] configfs-tsm: Attestation Report ABI Dan Williams
2023-10-13 2:14 ` [PATCH v6 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target Dan Williams
2023-10-13 2:14 ` [PATCH v6 2/7] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-10-13 2:14 ` [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-10-13 4:43 ` Dionna Amalie Glaze
2023-10-13 5:15 ` Dan Williams
2023-10-16 6:36 ` Alexey Kardashevskiy
2023-10-17 2:19 ` Dan Williams
2023-10-17 6:20 ` Alexey Kardashevskiy
2023-10-19 1:29 ` Dan Williams
2023-10-19 20:24 ` Dan Williams
2023-10-13 2:14 ` [PATCH v6 4/7] virt: sevguest: Prep for kernel internal get_ext_report() Dan Williams
2023-10-13 2:14 ` [PATCH v6 5/7] mm/slab: Add __free() support for kvfree Dan Williams
2023-10-13 2:14 ` [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
2023-10-13 15:38 ` Tom Lendacky
2023-10-14 4:46 ` Dan Williams
2023-10-16 11:36 ` Alexey Kardashevskiy
2023-10-16 15:39 ` Dionna Amalie Glaze
2023-10-16 15:42 ` Peter Gonda
2023-10-17 0:42 ` Alexey Kardashevskiy
2023-10-19 4:30 ` Dan Williams
2023-10-17 4:07 ` Dan Williams
2023-10-17 5:35 ` Alexey Kardashevskiy
2023-10-17 6:28 ` Alexey Kardashevskiy
2023-10-19 4:43 ` Dan Williams [this message]
2023-10-19 5:12 ` Alexey Kardashevskiy
2023-10-19 3:34 ` Dan Williams
2023-10-13 2:14 ` [PATCH v6 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
2023-10-19 18:12 ` Peter Gonda
2023-10-13 15:39 ` [PATCH v6 0/7] configfs-tsm: Attestation Report ABI Tom Lendacky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6530b3fda1e2d_5c0d29451@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=aik@amd.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=dave.hansen@linux.intel.com \
--cc=dionnaglaze@google.com \
--cc=jpiotrowski@linux.microsoft.com \
--cc=linux-coco@lists.linux.dev \
--cc=peterz@infradead.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=thomas.lendacky@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.