Re: [PATCH 10/17] prmem: documentation

[Nadav Amit <nadav.amit@gmail.com>]

> On Nov 21, 2018, at 8:34 AM, Igor Stoppa <igor.stoppa@gmail.com> wrote:
> 
> Hi,
> 
> On 13/11/2018 20:36, Andy Lutomirski wrote:
>> On Tue, Nov 13, 2018 at 10:33 AM Igor Stoppa <igor.stoppa@huawei.com> wrote:
>>> I forgot one sentence :-(
>>> 
>>> On 13/11/2018 20:31, Igor Stoppa wrote:
>>>> On 13/11/2018 19:47, Andy Lutomirski wrote:
>>>> 
>>>>> For general rare-writish stuff, I don't think we want IRQs running
>>>>> with them mapped anywhere for write.  For AVC and IMA, I'm less sure.
>>>> 
>>>> Why would these be less sensitive?
>>>> 
>>>> But I see a big difference between my initial implementation and this one.
>>>> 
>>>> In my case, by using a shared mapping, visible to all cores, freezing
>>>> the core that is performing the write would have exposed the writable
>>>> mapping to a potential attack run from another core.
>>>> 
>>>> If the mapping is private to the core performing the write, even if it
>>>> is frozen, it's much harder to figure out what it had mapped and where,
>>>> from another core.
>>>> 
>>>> To access that mapping, the attack should be performed from the ISR, I
>>>> think.
>>> 
>>> Unless the secondary mapping is also available to other cores, through
>>> the shared mm_struct ?
>> I don't think this matters much.  The other cores will only be able to
>> use that mapping when they're doing a rare write.
> 
> 
> I'm still mulling over this.
> There might be other reasons for replicating the mm_struct.
> 
> If I understand correctly how the text patching works, it happens sequentially, because of the text_mutex used by arch_jump_label_transform
> 
> Which might be fine for this specific case, but I think I shouldn't introduce a global mutex, when it comes to data.
> Most likely, if two or more cores want to perform a write rare operation, there is no correlation between them, they could proceed in parallel. And if there really is, then the user of the API should introduce own locking, for that specific case.

I think that if you create per-CPU temporary mms as you proposed, you do not
need a global lock. You would need to protect against module unloading, and
maybe refactor the code. Specifically, I’m not sure whether protection
against IRQs is something that you need. I’m also not familiar with this
patch-set so I’m not sure what atomicity guarantees do you need.

> A bit unrelated question, related to text patching: I see that each patching operation is validated, but wouldn't it be more robust to first validate  all of then, and only after they are all found to be compliant, to proceed with the actual modifications?
> 
> And about the actual implementation of the write rare for the statically allocated variables, is it expected that I use Nadav's function?

It’s not “my” function. ;-)

IMHO the code is in relatively good and stable state. The last couple of
versions were due to me being afraid to add BUG_ONs as Peter asked me to.

The code is rather simple, but there are a couple of pitfalls that hopefully
I avoided correctly.

Regards,
Nadav