Messages in this thread | | | Subject | Re: [PATCH Part1 RFC v4 15/36] x86/mm: Add support to validate memory when changing C-bit | From | Brijesh Singh <> | Date | Tue, 17 Aug 2021 13:07:40 -0500 |
| |
On 8/17/21 12:27 PM, Borislav Petkov wrote: > > The majority of kernel code puts __packed after the struct definition, > let's put it there too pls, out of the way. > > ...
Noted.
>> + if (WARN(ret || ghcb->save.sw_exit_info_2, >> + "SEV-SNP: page state change failed ret=%d exit_info_2=%llx\n", >> + ret, ghcb->save.sw_exit_info_2)) >> + return 1; > > Yikes, you return here and below with interrupts disabled. > > All your returns need to be "goto out;" instead where you do > > out: > __sev_put_ghcb(&state); > local_irq_restore(flags); > > Yap, you very likely need to put the GHCB too. >
Sure, let me revisit this code to fix those path.
>> + /* >> + * Lets do some sanity check that entry processing is not going >> + * backward. This will happen only if hypervisor is tricking us. >> + */ >> + if (WARN((hdr->end_entry > end_entry) || (cur_entry > hdr->cur_entry), >> + "SEV-SNP: page state change processing going backward, end_entry " >> + "(expected %d got %d) cur_entry (expected %d got %d)\n", >> + end_entry, hdr->end_entry, cur_entry, hdr->cur_entry)) >> + return 1; > > WARNING: quoted string split across lines > #293: FILE: arch/x86/kernel/sev.c:750: > + "SEV-SNP: page state change processing going backward, end_entry " > + "(expected %d got %d) cur_entry (expected %d got %d)\n", > > If you're wondering what to do, yes, you can really stretch that string > and shorten it too:
Okay.
> > if (WARN((hdr->end_entry > end_entry) || (cur_entry > hdr->cur_entry), > "SEV-SNP: PSC processing going backwards, end_entry %d (got %d) cur_entry: %d (got %d)\n", > end_entry, hdr->end_entry, cur_entry, hdr->cur_entry)) > return 1; > > so that it fits on a single line and grepping can find it. > Noted.
>> + /* Lets verify that reserved bit is not set in the header*/ >> + if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n")) > > psc_entry has a ->reserved field too and since we're iterating over the > entries... > Sure I can add that check.
>> + >> + desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT); > > kzalloc() so that you don't have to memset() later in > __set_page_state().
Depending on the size, the __set_page_state() can be call multiple times so it should clear the desc memory before filling it.
> >> + if (!desc) >> + panic("failed to allocate memory"); > > Make that error message more distinctive so that *if* it happens, one > can pinpoint the place in the code where the panic comes from. >
Now I am running checkpatch and notice that it complain about the message too. I can add a BUG() or WARN() to get the stack trace before the crashing.
>> + while (vaddr < vaddr_end) { >> + /* >> + * Calculate the last vaddr that can be fit in one >> + * struct snp_psc_desc. >> + */ >> + next_vaddr = min_t(unsigned long, vaddr_end, >> + (VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr); >> + >> + __set_page_state(desc, vaddr, next_vaddr, op); >> + >> + vaddr = next_vaddr; >> + } >> + >> + kfree(desc); >> +} >> + >
| |