Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 03/11/15 21:03, Thomas Huth wrote:

> On 03/11/15 20:21, Mark Cave-Ayland wrote:
>> On 03/11/15 15:23, Thomas Huth wrote:
>>
>>> On 23/10/15 15:56, Mark Cave-Ayland wrote:
>>>> From: Alexander Graf <agraf@suse.de>
>>>>
>>>> The lsxw instruction checks whether the desired string actually fits
>>>> into all defined registers. Unfortunately it does the calculation wrong,
>>>> resulting in illegal instruction traps for loads that really should fit.
>>>
>>> s/lsxw/lswx/ in the above text and in the title ... but I guess this
>>> could also be done when this patch gets picked up.
>>>
>>>> Fix it up, making Mac OS happier.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  target-ppc/mem_helper.c |    5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>>>> index 6d37dae..7e1f234 100644
>>>> --- a/target-ppc/mem_helper.c
>>>> +++ b/target-ppc/mem_helper.c
>>>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
>>>>                   uint32_t ra, uint32_t rb)
>>>>  {
>>>>      if (likely(xer_bc != 0)) {
>>>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
>>>> -                     (reg < rb && (reg + xer_bc) > rb))) {
>>>> +        int num_used_regs = (xer_bc + 3) / 4;
>>>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
>>>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>>>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>>>>                                         POWERPC_EXCP_INVAL |
>>>>                                         POWERPC_EXCP_INVAL_LSWX);
>>>
>>> The calculation of num_used_regs looks fine to me ... but is the
>>> remaining part of the condition really right already?
>>>
>>> According to the PowerISA:
>>>
>>>  If RA or RB is in the range of registers to be loaded,
>>>  including the case in which RA=0, the instruction is
>>>  treated as if the instruction form were invalid. If RT=RA
>>>  or RT=RB, the instruction form is invalid.
>>>
>>> So I wonder whether the check for "ra != 0" is really necessary here?
>>> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
>>> ra"? And "reg <= rb", too, of course?
>>>
>>> Also this code seems to completely ignore the case of the register
>>> wrap-around, where the sequence of registers jumps back to r0 ...
>>>
>>> So I'm basically fine with the num_used_regs fix for now, but I think
>>> this needs a big "FIXME" comment so that the remaining issues get
>>> cleaned up later?
>>
>> This was one of Alex's patches that was originally queued for ppc-next
>> before being dropped for missing the SoB, so I was expecting review to
>> find issues with other patches in the set rather than this one...
>>
>> Having said that, I'm not sure whether this was deliberate for
>> compatibility reasons or just an oversight. Unless David has any ideas
>> it might be that we have to wait for Alex to return before this series
>> can be included, but thanks for the review anyhow :)
> 
> Well, as I said, I'm basically fine with the patch, since it fixes one
> bug and the fix itself also looks fine. It's just that the surrounding
> code looks like it contains some more bugs... but these could also be
> fixed with a later patch, I guess.

Okay, great. I've fixed the comments in my local branch and will
resubmit on-list if David is happy with the changes.


Many thanks,

Mark.