On 14/06/2021 18:25, Fabiano Rosas wrote: > "Bruno Larsen (billionai)" writes: > >> This patch changes ppc_cpu_get_phys_page_debug so that it is now >> able to translate both, priviledged and real mode addresses >> independently of whether the CPU executing it has those permissions >> >> This was mentioned by Fabiano as something that would be very useful to >> help with debugging, but could possibly constitute a security issue if >> that debug function can be called in some way by prodution code. > Thinking a bit more about this, I think we just need to make sure that > this is not called during the regular operation of the virtual > machine. So not as much a security issue, more of a correctness one. yeah, but it's an issue of correctness that can lead to a security issue, so I think it's worth documenting at the very least > >> the >> solution was implemented such that it would be trivial to wrap it around >> ifdefs for building only with --enable-debug, for instance, but we are >> not sure this is the best approach, hence why it is an RFC. >> >> Suggested-by: Fabiano Rosas >> Signed-off-by: Bruno Larsen (billionai) >> --- >> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c >> index 9dcdf88597..41c727c690 100644 >> --- a/target/ppc/mmu_helper.c >> +++ b/target/ppc/mmu_helper.c >> @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) >> cpu_mmu_index(&cpu->env, true), false)) { >> return raddr & TARGET_PAGE_MASK; >> } >> + >> + /* >> + * This is a fallback, in case we're asking for priviledged memory to >> + * be printed, but the PCU is not executing in a priviledged manner. >> + * >> + * The code could be considered a security vulnerability if >> + * this function can be called in a scenario that does not involve >> + * debugging. >> + * Given the name and how useful using real addresses may be for >> + * actually debugging, however, we decided to include it anyway and >> + * discuss how to best avoid the possible security concerns. >> + * The current plan is that, if there is a chance this code is called in >> + * a production environment, we can surround it with ifdefs so that it >> + * is only compiled with --enable-debug >> + */ >> + /* attempt to translate first with virtual addresses */ >> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) || >> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) || >> + /* if didn't work, attempt to translate with real addresses */ >> + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) || >> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) { >> + return raddr & TARGET_PAGE_MASK; >> + } > If this is only used during debug we could just give it the highest > mmu_idx, no need for a fallback. we actually don't want to set the HV bit, because gdb is using the virtual hypervisor, so it'd trigger an assert that both HV and vhyp are set. > > Now, it might be possible that people use GDB to debug some aspect of > the MMU emulation, in which case it would be more useful to have the GDB > access fail just as the CPU would. But from my perspective it would be > better to have GDB access all of the guest memory without restriction. Yeah, could also be a thing. I really don't know, though, because before the mmu_idx was 0, so it wouldn't work even before this patch. Some more discussion is appreciated for the people who implement MMUs :) > >> return -1; >> } -- Bruno Piazera Larsen Instituto de Pesquisas ELDORADO Departamento Computação Embarcada Analista de Software Trainee Aviso Legal - Disclaimer