Coder Social home page Coder Social logo

Comments (22)

dubek avatar dubek commented on June 14, 2024 1

BTW in another place in the spec (page 115) it says: A: Bytes 30h to 5Fh of the request message (exactly like the kernel module implementation).

from linux-svsm.

dubek avatar dubek commented on June 14, 2024 1

Have you tried making msg and msg_resp page-aligned?

In sev-guest we have:

	/* Allocate the shared page used for the request and response message. */
	snp_dev->request = alloc_shared_pages(sizeof(struct snp_guest_msg));
	if (!snp_dev->request)
		goto e_unmap;

	snp_dev->response = alloc_shared_pages(sizeof(struct snp_guest_msg));
	if (!snp_dev->response)
		goto e_free_request;

though I'd expect a different error code if that was the problem:

The firmware checks that REQUEST_PADDR and RESPONSE_PADDR are valid sPAs. The
firmware checks that the response message will not cross a 4kB system physical page boundary
when written. If either of these checks fail, the firmware returns INVALD_ADDRESS.

[typo in the spec]

from linux-svsm.

dubek avatar dubek commented on June 14, 2024 1

It seems that pgtable_make_pages_shared zeros the page after marking it as shared.

@arkivm can you try moving the calls to pgtalbe_make_pages_shared to the beginning, before you start populating the fields of msg ? Or maybe populate msg and then copy it over to another page that you mark as shared? (this mimic what the sev-guest kernel module does)

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024 1

It seems that pgtable_make_pages_shared zeros the page after marking it as shared.

Ah, good point. The pgtable_make_pages_shared() function does not decrypt-in-place, it merely changes the c-bit and since any data present would then be non-sensical at that point, the memory is zeroed.

Also, be sure to call pgtable_make_pages_private() before freeing the memory.

@arkivm can you try moving the calls to pgtalbe_make_pages_shared to the beginning, before you start populating the fields of msg ? Or maybe populate msg and then copy it over to another page that you mark as shared? (this mimic what the sev-guest kernel module does)

from linux-svsm.

dbuono avatar dbuono commented on June 14, 2024 1

Would it make more sense to call pgtable_make_pages_shared and pgtable_make_pages_private in the constructors and destructors of the message types?
If someone forgets to make it private again, these pages may be eventually be reused and possibly cause a significant leak

from linux-svsm.

dubek avatar dubek commented on June 14, 2024

One more Q: the spec says IV is 96-bit but your call to wc_AesGcmEncrypt passes core::mem::size_of::<[u64; 2]>() as u32, // sizeof(iv) which is 16 (= 128-bit). Is that OK?

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Have you tried making msg and msg_resp page-aligned?

Sorry that I couldn't post the whole patch. The is a ton of crap there.
Yes. This is how i define it in the headers.

struct snp_guest_msg {
        struct snp_guest_msg_hdr hdr;
        u8 payload[4000];
} __attribute__((aligned(4096)));

Also from the kvm-trace, I see they are page-aligned.

 qemu-system-x86-618637 [201] 460835.696701: kvm_vmgexit_enter:    vcpu 0, exit_reason 80000011, exit_info1 80001d4000, exit_info2 80001d6000
 qemu-system-x86-618637 [201] 460835.697663: kvm_vmgexit_exit:     vcpu 0, exit_reason 80000011, exit_info1 0, exit_info2 16

which is 16 (= 128-bit). Is that OK?

I did try both 12 bytes and 16 bytes. The error persists.

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

From the link in the first comment...

Are you incrementing the sequence number before the call to the ASP/PSP? It is only incremented after a successful call to the ASP/PSP and incremented by 2.

Also, the VMPL must correspond to the key being used, to VMPL 0 must be specified.

Take a look at the sev-guest driver: https://elixir.bootlin.com/linux/latest/source/drivers/virt/coco/sev-guest/sev-guest.c

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Also, the VMPL must correspond to the key being used, to VMPL 0 must be specified.

The spec says,

The guest may generate attestation reports for VMPLs that are greater than or equal to the current VMPL.

So, technically passing any vmpl (0, 1, 2) from SVSM should work. no?

Just to not rule this out, I have tried all the levels. It is fixated on error 0x16.

Are you incrementing the sequence number before the call to the ASP/PSP? It is only incremented after a successful call to the ASP/PSP and incremented by 2.

The kernel driver sets a seq_no on the request (n), the firmware increments it by 1 on the response (n+1). The next request from the guest has n+2. In my code snippet, I pass n=1 for my request (as this is my first req). I did not bother to increment it yet, as the firmware request did not succeed.

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

Also, the VMPL must correspond to the key being used, to VMPL 0 must be specified.

The spec says,

The guest may generate attestation reports for VMPLs that are greater than or equal to the current VMPL.

So, technically passing any vmpl (0, 1, 2) from SVSM should work. no?

Yes, I missed that part in the spec.

Just to not rule this out, I have tried all the levels. It is fixated on error 0x16.

Are you incrementing the sequence number before the call to the ASP/PSP? It is only incremented after a successful call to the ASP/PSP and incremented by 2.

The kernel driver sets a seq_no on the request (n), the firmware increments it by 1 on the response (n+1). The next request from the guest has n+2. In my code snippet, I pass n=1 for my request (as this is my first req). I did not bother to increment it yet, as the firmware request did not succeed.

Yup, my bad, it is one more than the current value.

Can you print out the various values that are being initialized along with their offsets that will be passed through to the PSP? Like the header version, header size, etc.

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Can you print out the various values that are being initialized along with their offsets that will be passed through to the PSP? Like the header version, header size, etc.

IV { 01 00 }
hdr snp_guest_msg_hdr {
offset 0
 - authtag
44 b8 46 a0 06 46 10 a4 91 78 1d 83 c5 69 21 0c
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
offset 20
 - msg_seqno 1
offset 30
 - algo 1
offset 31
 - hdr_version 1
offset 32
 - hdr_sz 96
offset 34
 - msg_type 5
offset 35
 - msg_version 1
offset 36
 - msg_sz 96
offset 3c
 - msg_vmpck 0
}

Length: 96 (0x60) bytes
0000:   44 b8 46 a0  06 46 10 a4  91 78 1d 83  c5 69 21 0c   D.F..F...x...i!.
0010:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0020:   01 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0030:   01 01 60 00  05 01 60 00  00 00 00 00  00 00 00 00   ..`...`.........
0040:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0050:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................

Below is from the guest kernel's sevguest module for comparison.
Obviously, the offsets 0x20 (seq_no), 0x3c (vmpck) vary.

[  266.861304] snp-guest snp-guest: algo 0x1 hdr_version 0x1 hdr_sz 0x60 msg_type 0x5 msg_version 0x1 seqno 0x5 vmpck 0x1 msg_sz 0x60 crypto->iv_len 0xc crypto->a_len 0x10
[  266.861306] snp-guest snp-guest: snp_guest_msg_hdr:
[  266.861307] sevguest 00000000: 36 b8 da 0e af 00 ed db 86 5b 03 87 3f 5c 46 91
[  266.861307] sevguest 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  266.861308] sevguest 00000020: 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  266.861309] sevguest 00000030: 01 01 60 00 05 01 60 00 00 00 00 00 01 00 00 00
[  266.861309] sevguest 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  266.861310] sevguest 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

So the header looks correct, how about the message payload, can you print that?

Here is what the SEV guest driver looks like before and after the payload is encrypted:

[  664.969717] REQ (before): 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.971868] REQ (before): 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.973237] REQ (before): 00000020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.974661] REQ (before): 00000030: 01 01 60 00 05 01 60 00 00 00 00 00 00 00 00 00  ..`...`.........
[  664.978037] REQ (before): 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.980419] REQ (before): 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.982174] REQ (before): 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.984035] REQ (before): 00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.985622] REQ (before): 00000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.988004] REQ (before): 00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.989971] REQ (before): 000000a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.991616] REQ (before): 000000b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.993696] REQ  (after): 00000000: ff 16 d9 0f 20 f2 67 66 fe 1a eb a1 bd d1 d7 9d  .... .gf........
[  664.995707] REQ  (after): 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.997488] REQ  (after): 00000020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  664.999469] REQ  (after): 00000030: 01 01 60 00 05 01 60 00 00 00 00 00 00 00 00 00  ..`...`.........
[  665.001428] REQ  (after): 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  665.002829] REQ  (after): 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  665.005184] REQ  (after): 00000060: 14 a7 b3 68 dd c2 6b 4b e0 15 b9 e9 30 de b8 28  ...h..kK....0..(
[  665.006620] REQ  (after): 00000070: 94 db 87 f9 d1 f5 a3 14 2d 18 a5 58 e4 44 46 75  ........-..X.DFu
[  665.008661] REQ  (after): 00000080: 37 5e 68 3c be 93 14 48 d3 f2 e8 61 7e 32 d6 81  7^h<...H...a~2..
[  665.010685] REQ  (after): 00000090: d6 91 ad 83 3d 93 65 3c 8a 19 65 d4 59 9a ec 43  ....=.e<..e.Y..C
[  665.012757] REQ  (after): 000000a0: 82 0a 22 07 25 ce 3c 1b 23 a4 cb 10 23 56 9e aa  ..".%.<.#...#V..
[  665.014188] REQ  (after): 000000b0: 7d b2 34 ea 0f e4 e2 d8 b6 73 b7 33 1a 7f 4a 55  }.4......s.3..JU

And if that all looks correct, can you enable the CCP debug support to ensure that the PSP call is actually being made and not returning early for some reason: echo "module ccp +p" > /proc/dynamic_debug/control

You should see something in the host/hypervisor dmesg like:

[330240.595246] ccp 0000:23:00.1: sev command id 0x94 buffer 0x000000016e950000 timeout 100s
[330240.595261] (in):  00000000: 9000 a68c 0010 0000 4000 68fa 0014 0000
[330240.595263] (in):  00000010: a000 4f0a 0014 0000
[330240.602663] (out): 00000000: 9000 a68c 0010 0000 4000 68fa 0014 0000
[330240.602667] (out): 00000010: a000 4f0a 0014 0000
[330240.602670] ccp 0000:23:00.1: sev command id 0xc7 buffer 0x000000016e950000 timeout 100s
[330240.602673] (in):  00000000: a000 4f0a 0014 0000
[330240.603027] (out): 00000000: a000 4f0a 0014 0000

from linux-svsm.

dubek avatar dubek commented on June 14, 2024

Do you get any other error besides INVALID_PARAM ? For example, if you deliberately corrupt the authtag, do you get a BAD_MEASUREMENT error? If you mess the sequence number do you get AEAD_OFLOW?
(I'm getting these error codes from section 8.26.2 in the SNP ABI spec.)

Another experiment might be to try a different guest message. For example MSG_TSC_INFO_REQ (=17 decimal), whose message payload is 128 zero bytes.

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Do you get any other error besides INVALID_PARAM ?

Unfortunately not!

  • corrupted authtag -> 0x16
  • SNP_GUEST_REQUEST without making the request page shared before ghcb request -> 0x16
  • bogus seqno -> 0x16
    • However, if I give a bogus seqno inside the guest kernel, I get AEAD_OFLOW - 0x1D
  • populating a reserved field in the header -> 0x16

So the header looks correct, how about the message payload, can you print that?

To confirm if the generated authtag is correct, I did the following test.

  • Use vmpck1 instead of vmpck0 to encrypt the payload in both svsm and guest application
  • Both the hexdump matches

Requesting report from SVSM - using vmpck1 and req.vmpl = 1

Enc key: Length: 32 (0x20) bytes
0000:   fe 24 6a dd  1a c3 39 15  ee 4b f8 6e  86 9b 0b a5   .$j...9..K.n....
0010:   72 bd 33 f2  6e 88 34 04  d7 c7 0c ca  0b 40 b4 b3   r.3.n.4......@..
IV: Length: 12 (0xc) bytes
0000:   01 00 00 00  00 00 00 00  00 00 00 00                ............
AAD: Length: 48 (0x30) bytes
0000:   01 01 60 00  05 01 60 00  00 00 00 00  01 00 00 00   ..`...`.........
0010:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0020:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
Payload
Length: 96 (0x60) bytes
0000:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0010:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0020:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0030:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0040:   01 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0050:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
--- end ---
Before enc
Length: 192 (0xc0) bytes
0000:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0010:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0020:   01 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0030:   01 01 60 00  05 01 60 00  00 00 00 00  01 00 00 00   ..`...`.........
0040:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0050:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0060:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0070:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0080:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0090:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
00a0:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
00b0:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
--- end ---
After enc
Length: 192 (0xc0) bytes
0000:   ee 85 8c 71  f0 18 16 2d  b6 82 ec ee  72 44 a4 cd   ...q...-....rD..
0010:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0020:   01 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0030:   01 01 60 00  05 01 60 00  00 00 00 00  01 00 00 00   ..`...`.........
0040:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0050:   00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00   ................
0060:   a1 a1 56 d6  2d 7e b5 8a  86 77 e5 fb  f3 8a ad 4c   ..V.-~...w.....L
0070:   c3 dc 93 22  e1 6e d0 2e  32 4e 74 05  80 b7 6b f7   ...".n..2Nt...k.
0080:   c6 71 cb 56  fb 03 0a f6  b3 45 69 26  77 e7 23 1e   .q.V.....Ei&w.#.
0090:   f2 10 0d 39  ef 12 b8 66  70 1d ad cf  a9 5a a7 66   ...9...fp....Z.f
00a0:   92 06 16 db  36 aa 88 30  89 e7 ec ab  d1 64 0b 9a   ....6..0.....d..
00b0:   77 2a 78 88  58 e9 bf d7  f1 22 08 8f  16 3a f2 ad   w*x.X...."...:..
--- end ---

And if that all looks correct, can you enable the CCP debug support to ensure that the PSP call is actually being made

  • host ccp log for the above SVSM request
[684408.748592] ccp 0000:22:00.1: sev command id 0x94 buffer 0x000000011825e000 timeout 100s
[684408.748600] (in):  00000000: 1000 5ddb 0001 0000 2000 7ffd 0005 0000
[684408.748602] (in):  00000010: 4000 7ffd 0005 0000
[684408.749499] ccp 0000:22:00.1: sev command 0x94 failed (0x00000016)
[684408.749502] (out): 00000000: 1000 5ddb 0001 0000 2000 7ffd 0005 0000
[684408.749504] (out): 00000010: 4000 7ffd 0005 0000
[684408.749505] ccp 0000:22:00.1: sev command id 0xc7 buffer 0x000000011825e000 timeout 100s
[684408.749508] (in):  00000000: 4000 7ffd 0005 0000
[684408.749849] (out): 00000000: 4000 7ffd 0005 0000

Requesting report from guest application - using vmpck1 and req.vmpl = 1

Key: 00000000: fe 24 6a dd 1a c3 39 15 ee 4b f8 6e 86 9b 0b a5
Key: 00000010: 72 bd 33 f2 6e 88 34 04 d7 c7 0c ca 0b 40 b4 b3

snp-guest snp-guest: Initialized SNP guest driver (using vmpck_id 1)
snp-guest snp-guest: request [seqno 1 type 5 version 1 sz 96]

sevguest IV 00000000: 01 00 00 00 00 00 00 00 00 00 00 00

sevguest AAD 00000000: 01 01 60 00 05 01 60 00 00 00 00 00 01 00 00 00
sevguest AAD 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest AAD 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

sevguest Payload 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest Payload 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest Payload 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest Payload 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest Payload 00000040: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest Payload 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

sevguest REQ (before enc) 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000030: 01 01 60 00 05 01 60 00 00 00 00 00 01 00 00 00
sevguest REQ (before enc) 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 000000a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (before enc) 000000b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

sevguest REQ (after enc) 00000000: ee 85 8c 71 f0 18 16 2d b6 82 ec ee 72 44 a4 cd
sevguest REQ (after enc) 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (after enc) 00000020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (after enc) 00000030: 01 01 60 00 05 01 60 00 00 00 00 00 01 00 00 00
sevguest REQ (after enc) 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (after enc) 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
sevguest REQ (after enc) 00000060: a1 a1 56 d6 2d 7e b5 8a 86 77 e5 fb f3 8a ad 4c
sevguest REQ (after enc) 00000070: c3 dc 93 22 e1 6e d0 2e 32 4e 74 05 80 b7 6b f7
sevguest REQ (after enc) 00000080: c6 71 cb 56 fb 03 0a f6 b3 45 69 26 77 e7 23 1e
sevguest REQ (after enc) 00000090: f2 10 0d 39 ef 12 b8 66 70 1d ad cf a9 5a a7 66
sevguest REQ (after enc) 000000a0: 92 06 16 db 36 aa 88 30 89 e7 ec ab d1 64 0b 9a
sevguest REQ (after enc) 000000b0: 77 2a 78 88 58 e9 bf d7 f1 22 08 8f 16 3a f2 ad
  • and the host ccp debug log for this request
[684449.241427] ccp 0000:22:00.1: sev command id 0x94 buffer 0x000000011825e000 timeout 100s
[684449.241436] (in):  00000000: 1000 5ddb 0001 0000 2000 8199 0007 0000
[684449.241439] (in):  00000010: 1000 8199 0007 0000
[684449.248614] (out): 00000000: 1000 5ddb 0001 0000 2000 8199 0007 0000
[684449.248618] (out): 00000010: 1000 8199 0007 0000
[684449.248621] ccp 0000:22:00.1: sev command id 0xc7 buffer 0x000000011825e000 timeout 100s
[684449.248625] (in):  00000000: 1000 8199 0007 0000
[684449.248976] (out): 00000000: 1000 8199 0007 0000

The only difference I could spot is the address of the request and response buffers. Any restrictions on those? I have shared them using pgtable_make_pages_shared function.


Another experiment might be to try a different guest message. For example MSG_TSC_INFO_REQ (=17 decimal), whose message payload is 128 zero bytes.

Got 0x16 for this too.

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

Can you try dumping the RMP entry for each of the request and response addresses and the RMP entry for the 2MB alignment of each (snp_lookup_rmpentry()). Additionally, enable the KVM page state change trace point (kvm_snp_psc). That may give us some clues as to whether the pages are being properly converted.

In the meantime, I'll try and see if I can duplicate the issue locally (though it may not be until after Thanksgiving).

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Additionally, enable the KVM page state change trace point (kvm_snp_psc). That may give us some clues as to whether the pages are being properly converted.

 kvm_vmgexit_enter:    vcpu 0, exit_reason 80000010, exit_info1 0, exit_info2 0
 kvm_snp_psc:          vcpu 0, pfn 21c37d2, gpa 80001d2000, op 0x2, level 1
 kvm_vmgexit_exit:     vcpu 0, exit_reason 80000010, exit_info1 0, exit_info2 0
--
 kvm_vmgexit_enter:    vcpu 0, exit_reason 80000010, exit_info1 0, exit_info2 0
 kvm_snp_psc:          vcpu 0, pfn 21c37d4, gpa 80001d4000, op 0x2, level 1
 kvm_vmgexit_exit:     vcpu 0, exit_reason 80000010, exit_info1 0, exit_info2 0
 kvm_entry:            vcpu 0, rip 0x0
 kvm_exit:             reason UNKNOWN (1027) rip 0x0 info 0 0
 kvm_vmgexit_enter:    vcpu 0, exit_reason 80000011, exit_info1 80001d2000, exit_info2 80001d4000
 kvm_vmgexit_exit:     vcpu 0, exit_reason 80000011, exit_info1 0, exit_info2 16

I get psc requests for both req and resp buffers from svsm and also the exit_info2 as 0x16 (failure).

I will try dumping the RMP entries.

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

It seems that pgtable_make_pages_shared zeros the page after marking it as shared.

Wow. Thanks @dubek. That was a bit unexpected. My bad. I overlooked this.

Marking it as shared early on solves the firmware returning 0x16 issue. I am trying to decrypt and parse the report now.

since any data present would then be non-sensical at that point, the memory is zeroed.

To avoid leaking, we can follow @dbuono suggestion. But to avoid stepping onto this landmine again, does it make sense to return an error if the page is non-zero? (for both pgtable_make_pages_shared and pgtable_make_pages_private).

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

To avoid leaking, we can follow @dbuono suggestion. But to avoid stepping onto this landmine again, does it make sense to return an error if the page is non-zero? (for both pgtable_make_pages_shared and pgtable_make_pages_private).

No, I don't believe that is necessary. Also, pgtable_make_pages_shared/private should only be called at a level that you know you won't affect other data. So really, you should be using mem_allocate_frame/frames to allocate isolated pages that you will work on.

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Also, pgtable_make_pages_shared/private should only be called at a level that you know you won't affect other data. So really, you should be using mem_allocate_frame/frames to allocate isolated pages that you will work on.

Unfortunately, this is neither documented in the code, nor it is implicit from the function name. When I call a function that's named xxx_make_private/xxx_make_shared, nobody expects the function to trample on the data.

For instance, consider these functions pgtable_make_pages_nx / pgtable_make_pages_np - it does what it advertises. Just make the page NX, NP. Somehow users cannot magically assume that making it private/shared is going to memset their data.

from linux-svsm.

tlendacky avatar tlendacky commented on June 14, 2024

Unfortunately, this is neither documented in the code, nor it is implicit from the function name. When I call a function that's named xxx_make_private/xxx_make_shared, nobody expects the function to trample on the data.

Fair point on the documentation. But when there is lack of documentation you need to look at what the code does, not make an assumption of what the code does based on the name.

For instance, consider these functions pgtable_make_pages_nx / pgtable_make_pages_np - it does what it advertises. Just make the page NX, NP. Somehow users cannot magically assume that making it private/shared is going to memset their data.

Even without the memset you would have failed. Changing the encryption bit changes the view of the data in the page - it does not automatically convert the data in the page to the new form. If you create a structure with the encryption bit set and then change the bit, you get back ciphertext. Sure, the memset can be removed, but just like in the Linux kernel you are expected to do the memset() (or ensure you set every field) after calling set_memory_encrypted()/decrypted(). Even though the code is written in a high-level language, you are dealing with a very low level OS type environment that is manipulating GDTs, IDTs, pagetables, etc. and so you need to be aware of how all the functions operate when you use them.

from linux-svsm.

arkivm avatar arkivm commented on June 14, 2024

Sure, the memset can be removed,

Well, to clarify. I am not rooting for changing the behavior of this function. Changing the c-bit already makes the whole data useless as there is no decryption in-place.

make an assumption of what the code does based on the name.

Let's add more documentation and change the name (postfix it with _zeroed or something), as that would advertise what the function actually does.

from linux-svsm.

Zildj1an avatar Zildj1an commented on June 14, 2024

Sure, the memset can be removed,

Well, to clarify. I am not rooting for changing the behavior of this function. Changing the c-bit already makes the whole data useless as there is no decryption in-place.

make an assumption of what the code does based on the name.

Let's add more documentation and change the name (postfix it with _zeroed or something), as that would advertise what the function actually does.

I'm not a big fan of the readability of pgtable_make_pages_shared_zeroed(). As discussed, at this level of development we can't rely on the name of functions to understand their implications. But adding a comment in the function documentation would be nice.

from linux-svsm.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.