Messages in this thread | | | Date | Tue, 10 Oct 2023 12:36:39 -0700 | From | Dan Williams <> | Subject | Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS |
| |
Dan Williams wrote: > Peter Gonda wrote: > > On Tue, Oct 3, 2023 at 1:29 PM Kuppuswamy Sathyanarayanan > > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: [..] > > > Dan, do you think it is related to not allocating direct mapped memory (using > > > kvalloc)? > > > > But I think the issue is the stack allocated variable 'ext_req' here: > > > > sev_report_new() > > + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + guard(mutex)(&snp_cmd_mutex); > > + certs_address = buf + report_size; > > + struct snp_ext_report_req ext_req = { > > + .data = { .vmpl = desc->privlevel }, > > + .certs_address = (__u64)certs_address, > > + .certs_len = ext_size, > > + }; > > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len); > > If the failure is coming from: > > sg_set_buf(&src[1], src_buf, hdr->msg_sz); > > ...then that is always coming from the stack as get_ext_report() > internally copies either from the user ioctl() address or the kernel > stack into the local stack copy in both cases: > > get_ext_report(...) > ... > struct snp_ext_report_req req; > ... > if (copy_from_sockptr(&req, io->req_data, sizeof(req))) > return -EFAULT; > ... > ret = handle_guest_request(..., &req.data, ...); > > ...where that "&req.data" always becomes the @src_buf argument to > enc_dec_message(). So while I do understand why sg_set_buf() is > complaining, I don't understand why it is not *always* complaining, > regardless of configfs-tsm or ioctl() with CONFIG_DEBUG_SG=y builds. > > I will be able to dig deeper once I can test on hardware, but I am > thinking that the entire scheme to pass the source buffer on the kernel > stack is broken and is only happening to work because there are no > crypto-accelerators attached that require that the virtual addresses be > virt_addr_valid() for a later dma_map_sg() event. > > ...or my eyes are overlooking how the ioctl() path is succeeding.
Confirmed, I can make DEBUG_SG equally unhappy via the ioctl() path. Note I changed the BUG_ON() to WARN_ON() in this kernel to keep the system alive over reproductions:
WARNING: CPU: 0 PID: 1175 at include/linux/scatterlist.h:187 enc_dec_message+0x518/0x5b0 [sev_guest] [..] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023 RIP: 0010:enc_dec_message+0x518/0x5b0 [sev_guest] Call Trace: <TASK> [..] handle_guest_request+0x135/0x520 [sev_guest] get_ext_report+0x1ec/0x3e0 [sev_guest] snp_guest_ioctl+0x157/0x200 [sev_guest]
So the required fix here will address both cases.
I will note that on this instance no certificate data is being returned, so I can't test that path, but at least the report retrieval will be tested for the next posting of this series.
| |