| Message ID | 20210614191630.101304-2-bruno.larsen@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] target/ppc: fix address translation bug for radix mmus | expand |
On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote: > 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. 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 <farosas@linux.ibm.com> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > --- > 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 Nothing forbid us to use --enable-debug in a production environment. > + */ > + /* 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; > + } > return -1; > } > >
"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> 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. > 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 <farosas@linux.ibm.com> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > --- > 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. 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. > return -1; > }
On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: > 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. 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<farosas@linux.ibm.com> > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> > --- > target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) I think the first part is unnecessary. Either the cpu is in supervisor mode or it isn't, and gdb should use the correct address space. If you really want to force supervisor lookup from a guest that is paused in usermode, I suppose you could force MSR.PR=1 while you're performing the access and set it back afterward. I think the second part is actively wrong -- real-mode address lookup will (for the most part) always succeed. Moreover, the gdb user will have no idea that you've silently changed addressing methods. r~
On Mon, Jun 14, 2021 at 09:37:54PM +0200, Philippe Mathieu-Daudé wrote: > On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote: > > 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. 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 <farosas@linux.ibm.com> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > > --- > > 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. s/PCU/CPU/ > > + * > > + * 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 > > Nothing forbid us to use --enable-debug in a production environment. > > > + */ > > + /* 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; > > + } > > return -1; > > } > > > > >
On 14/06/2021 19:37, Richard Henderson wrote: > On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: >> 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. 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<farosas@linux.ibm.com> >> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> >> --- >> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) > > I think the first part is unnecessary. Either the cpu is in > supervisor mode or it isn't, and gdb should use the correct address > space. If you really want to force supervisor lookup from a guest > that is paused in usermode, I suppose you could force MSR.PR=1 while > you're performing the access and set it back afterward. I don't see why GDB should not be able to see supervisor level addresses just because the CPU can't. when debugging, we wanna see exactly what QEMU sees, not what the guest sees, right? Now, if this is changing more than just privilege level, I agree there is a problem, but I wouldn't think it is the case... > > I think the second part is actively wrong -- real-mode address lookup > will (for the most part) always succeed. Moreover, the gdb user will > have no idea that you've silently changed addressing methods. I disagree. Real-mode address will mostly fail, since during the boot process Linux kernels set the MMU to use only virtual addresses, so real mode addresses only work when debugging the firmware or the early setup of the kernel. After that, GDB can basically only see virtual addresses. Maybe there is a better way to handle this by having GDB warn the user that the CPU can not decode the address in it's current state, but I do think it is a good tool to have, as it would've made debugging the first RFC on this topic a bit easier, and farosas was actively complaining that isn't a feature yet. > > r~
On 14/06/2021 18:25, Fabiano Rosas wrote: > "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> 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 <farosas@linux.ibm.com> >> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> >> --- >> 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; >> }
On 14/06/2021 16:37, Philippe Mathieu-Daudé wrote: > On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote: >> 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. 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 <farosas@linux.ibm.com> >> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> >> --- >> 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 > Nothing forbid us to use --enable-debug in a production environment. True, but in general, running a debug build is considered a path to vulnerabilities one would consider to be avoidable. Also, I am not sure of a much better way to help devs that use QEMU without opening up to these kinds of info-leak vulnerabilities, since it's literally what we want the code to do. Having it require source code manipulation, like DO_CPU_STATISTICS did, could work, but it could also bit rot quickly. Definitely open to more discussion on the topic! :) > >> + */ >> + /* 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; >> + } >> return -1; >> } >> >>
On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote: > On 14/06/2021 19:37, Richard Henderson wrote: >> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: >>> 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. 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<farosas@linux.ibm.com> >>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> >>> --- >>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >> >> I think the first part is unnecessary. Either the cpu is in supervisor mode or it >> isn't, and gdb should use the correct address space. If you really want to force >> supervisor lookup from a guest that is paused in usermode, I suppose you could force >> MSR.PR=1 while you're performing the access and set it back afterward. > I don't see why GDB should not be able to see supervisor level addresses just because the > CPU can't. Because then when you are debugging, you then don't know whether the address is actually accessible in the current cpu context. >> I think the second part is actively wrong -- real-mode address lookup will (for the most >> part) always succeed. Moreover, the gdb user will have no idea that you've silently >> changed addressing methods. > > I disagree. Real-mode address will mostly fail, since during the boot process Linux > kernels set the MMU to use only virtual addresses, so real mode addresses only work when > debugging the firmware or the early setup of the kernel. After that, GDB can basically > only see virtual addresses. Exactly. But you changed that so that any unmapped address will re-try with real-mode, which (outside of hv) simply maps real->physical and returns the input. One should have to perform some special action to see addresses in a different cpu context. I don't think that gdb supports such a special action at the moment. If you want that feature though, that's where you should start. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote: >> On 14/06/2021 19:37, Richard Henderson wrote: >>> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: >>>> 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. 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<farosas@linux.ibm.com> >>>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> >>>> --- >>>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>> >>> I think the first part is unnecessary. Either the cpu is in supervisor mode or it >>> isn't, and gdb should use the correct address space. If you really want to force >>> supervisor lookup from a guest that is paused in usermode, I suppose you could force >>> MSR.PR=1 while you're performing the access and set it back afterward. >> I don't see why GDB should not be able to see supervisor level addresses just because the >> CPU can't. > > Because then when you are debugging, you then don't know whether the address is actually > accessible in the current cpu context. > @Bruno, so this is what I referred to somewhere else on the thread, people expect GDB to have the same access level of the currently executing code. So implementing my suggestion would break their workflow. >>> I think the second part is actively wrong -- real-mode address lookup will (for the most >>> part) always succeed. Moreover, the gdb user will have no idea that you've silently >>> changed addressing methods. >> >> I disagree. Real-mode address will mostly fail, since during the boot process Linux >> kernels set the MMU to use only virtual addresses, so real mode addresses only work when >> debugging the firmware or the early setup of the kernel. After that, GDB can basically >> only see virtual addresses. > > Exactly. But you changed that so that any unmapped address will re-try with real-mode, > which (outside of hv) simply maps real->physical and returns the input. > > One should have to perform some special action to see addresses in a different cpu > context. I don't think that gdb supports such a special action at the moment. If you > want that feature though, that's where you should start. I think we can just drop this patch. The scenarios where debugging across MMU contexts happen are quite limited. My use case was a while back when implementing single-step for KVM guests; there were some situations where GDB would have issues setting breakpoints around kernel code that altered MSR_IR/DR. But that is mostly anecdotal at this point. If I ever run into that again, now I know where to look. > > > r~
On Tue, Jun 15, 2021 at 08:32:32AM -0300, Bruno Piazera Larsen wrote: > On 14/06/2021 19:37, Richard Henderson wrote: > > On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: > > > 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. 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<farosas@linux.ibm.com> > > > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> > > > --- > > > target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > I think the first part is unnecessary. Either the cpu is in supervisor > > mode or it isn't, and gdb should use the correct address space. If you > > really want to force supervisor lookup from a guest that is paused in > > usermode, I suppose you could force MSR.PR=1 while you're performing the > > access and set it back afterward. > I don't see why GDB should not be able to see supervisor level addresses > just because the CPU can't. when debugging, we wanna see exactly what QEMU > sees, not what the guest sees, right? That kind of depends whether you mean gdb attached to the gdb socket provided by qemu - in which case I think you want it to see what the guest sees - or gdb debugging qemu itself, in which case it does want to see what qemu sees, but doesn't use this code path AFAIK. > Now, if this is changing more than > just privilege level, I agree there is a problem, but I wouldn't think it is > the case... > > I think the second part is actively wrong -- real-mode address lookup > > will (for the most part) always succeed. Moreover, the gdb user will > > have no idea that you've silently changed addressing methods. > > I disagree. Real-mode address will mostly fail, since during the boot > process Linux kernels set the MMU to use only virtual addresses, so real > mode addresses only work when debugging the firmware or the early setup of > the kernel. After that, GDB can basically only see virtual addresses. > > Maybe there is a better way to handle this by having GDB warn the user that > the CPU can not decode the address in it's current state, but I do think it > is a good tool to have, as it would've made debugging the first RFC on this > topic a bit easier, and farosas was actively complaining that isn't a > feature yet. > > > > > r~
On 15/06/2021 18:37, Fabiano Rosas wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote: >>> On 14/06/2021 19:37, Richard Henderson wrote: >>>> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote: >>>>> 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. 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<farosas@linux.ibm.com> >>>>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> >>>>> --- >>>>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>> I think the first part is unnecessary. Either the cpu is in supervisor mode or it >>>> isn't, and gdb should use the correct address space. If you really want to force >>>> supervisor lookup from a guest that is paused in usermode, I suppose you could force >>>> MSR.PR=1 while you're performing the access and set it back afterward. >>> I don't see why GDB should not be able to see supervisor level addresses just because the >>> CPU can't. >> Because then when you are debugging, you then don't know whether the address is actually >> accessible in the current cpu context. >> > @Bruno, so this is what I referred to somewhere else on the thread, > people expect GDB to have the same access level of the currently > executing code. So implementing my suggestion would break their > workflow. Ok, that makes sense. I'll drop this patch, then > >>>> I think the second part is actively wrong -- real-mode address lookup will (for the most >>>> part) always succeed. Moreover, the gdb user will have no idea that you've silently >>>> changed addressing methods. >>> I disagree. Real-mode address will mostly fail, since during the boot process Linux >>> kernels set the MMU to use only virtual addresses, so real mode addresses only work when >>> debugging the firmware or the early setup of the kernel. After that, GDB can basically >>> only see virtual addresses. >> Exactly. But you changed that so that any unmapped address will re-try with real-mode, >> which (outside of hv) simply maps real->physical and returns the input. >> >> One should have to perform some special action to see addresses in a different cpu >> context. I don't think that gdb supports such a special action at the moment. If you >> want that feature though, that's where you should start. > I think we can just drop this patch. The scenarios where debugging > across MMU contexts happen are quite limited. > > My use case was a while back when implementing single-step for KVM > guests; there were some situations where GDB would have issues setting > breakpoints around kernel code that altered MSR_IR/DR. But that is > mostly anecdotal at this point. If I ever run into that again, now I > know where to look. At least now it's documented how to make it work, if someone else ever needs it as well :) > >> >> r~
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; + } return -1; }
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. 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 <farosas@linux.ibm.com> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> --- target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)