linux-kernel - Re: [PATCH v2 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Apr 2020 14:53:45 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     mpe@...erman.id.au, mikey@...ling.org, apopple@...ux.ibm.com,
        paulus@...ba.org, npiggin@...il.com,
        naveen.n.rao@...ux.vnet.ibm.com, peterz@...radead.org,
        jolsa@...nel.org, oleg@...hat.com, fweisbec@...il.com,
        mingo@...nel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH v2 13/16] powerpc/watchpoint: Prepare handler to handle
 more than one watcnhpoint



On 4/1/20 2:50 PM, Christophe Leroy wrote:
> 
> 
> Le 01/04/2020 à 11:13, Ravi Bangoria a écrit :
>>
>>
>> On 4/1/20 12:20 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 01/04/2020 à 08:13, Ravi Bangoria a écrit :
>>>> Currently we assume that we have only one watchpoint supported by hw.
>>>> Get rid of that assumption and use dynamic loop instead. This should
>>>> make supporting more watchpoints very easy.
>>>>
>>>> With more than one watchpoint, exception handler need to know which
>>>> DAWR caused the exception, and hw currently does not provide it. So
>>>> we need sw logic for the same. To figure out which DAWR caused the
>>>> exception, check all different combinations of user specified range,
>>>> dawr address range, actual access range and dawrx constrains. For ex,
>>>> if user specified range and actual access range overlaps but dawrx is
>>>> configured for readonly watchpoint and the instruction is store, this
>>>> DAWR must not have caused exception.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
>>>> ---
>>>>   arch/powerpc/include/asm/processor.h |   2 +-
>>>>   arch/powerpc/include/asm/sstep.h     |   2 +
>>>>   arch/powerpc/kernel/hw_breakpoint.c  | 396 +++++++++++++++++++++------
>>>>   arch/powerpc/kernel/process.c        |   3 -
>>>>   4 files changed, 313 insertions(+), 90 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> -static bool
>>>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>>>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>>>> +                    struct arch_hw_breakpoint *info)
>>>>   {
>>>>       return ((dar <= info->address + info->len - 1) &&
>>>>           (dar + size - 1 >= info->address));
>>>>   }
>>>
>>> Here and several other places, I think it would be more clear if you could avoid the - 1 :
>>>
>>>      return ((dar < info->address + info->len) &&
>>>          (dar + size > info->address));
>>
>> Ok. see below...
>>
>>>
>>>
>>>> +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
>>>> +{
>>>> +    unsigned long hw_start_addr, hw_end_addr;
>>>> +
>>>> +    hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
>>>> +    hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE) - 1;
>>>> +
>>>> +    return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
>>>> +}
>>>
>>>      hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
>>>
>>>      return ((hw_start_addr <= dar) && (hw_end_addr > dar));
>>
>> I'm using -1 while calculating end address is to make it
>> inclusive. If I don't use -1, the end address points to a
>> location outside of actual range, i.e. it's not really an
>> end address.
> 
> But that's what is done is several places, for instance:
> 
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/mm/dma-noncoherent.c#L22
> 
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/include/asm/book3s/32/kup.h#L92
> 
> In several places like this, end is outside of the range. My feeling is that is helps with readability.

Agreed, it helps with readability. Will change it.
Thanks for the links.

Ravi