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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Oct 2023 12:36:39 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Dan Williams <dan.j.williams@...el.com>,
        Peter Gonda <pgonda@...gle.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        <linux-coco@...ts.linux.dev>, Erdem Aktas <erdemaktas@...gle.com>,
        <peterz@...radead.org>, <linux-kernel@...r.kernel.org>,
        <x86@...nel.org>, <dave.hansen@...ux.intel.com>
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@...ux.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.