Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Christophe Leroy]

Le 21/02/2024 à 18:30, Daniel Borkmann a écrit :
> On 2/19/24 7:39 AM, Christophe Leroy wrote:
>>
>>
>> Le 19/02/2024 à 02:40, Hengqi Chen a écrit :
>>> [Vous ne recevez pas souvent de courriers de hengqi.chen@xxxxxxxxx. 
>>> Découvrez pourquoi ceci est important à 
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hello Christophe,
>>>
>>> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy
>>> <christophe.leroy@xxxxxxxxxx> wrote:
>>>>
>>>> set_memory_ro() can fail, leaving memory unprotected.
>>>>
>>>> Check its return and take it into account as an error.
>>>>
>>>
>>> I don't see a cover letter for this series, could you describe how
>>> set_memory_ro() could fail.
>>> (Most callsites of set_memory_ro() didn't check the return values)
>>
>> Yeah, there is no cover letter because as explained in patch 2 the two
>> patches are autonomous. The only reason why I sent it as a series is
>> because the patches both modify include/linux/filter.h in two places
>> that are too close to each other.
>>
>> I should have added a link to https://github.com/KSPP/linux/issues/7
>> See that link for detailed explanation.
>>
>> If we take powerpc as an exemple, set_memory_ro() is a frontend to
>> change_memory_attr(). When you look at change_memory_attr() you see it
>> can return -EINVAL in two cases. Then it calls
>> apply_to_existing_page_range(). When you go down the road you see you
>> can get -EINVAL or -ENOMEM from that function or its callees.
> 
> By that logic, don't you have the same issue when undoing all of this?
> E.g. take arch_protect_bpf_trampoline() / arch_unprotect_bpf_trampoline()
> which is not covered in here, but what happens if you set it first to ro
> and later the setting back to rw fails? How would the error path there
> look like? It's something you cannot recover.
> 

arch_protect_bpf_trampoline() is handled there 
https://patchwork.kernel.org/project/netdevbpf/patch/883c5a268483a89ab13ed630210328a926f16e5b.1708526584.git.christophe.leroy@xxxxxxxxxx/

In case setting back to RW fails there is not security issue, the things 
will likely blow up later with a write access to write protected memory 
but in terms of security that's not a problem.