Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS

[Dan Williams]

Dan Williams wrote:
> Peter Gonda wrote:
> > On Tue, Oct 3, 2023 at 1:29 PM Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> 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.