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;
>>   }
>>   
>>
-- 
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>