linux-kernel - Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of cleaning up programs

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:   Mon, 12 Dec 2022 10:39:26 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tero Kristo <tero.kristo@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of
 cleaning up programs



On 12/12/22 10:20 AM, Greg KH wrote:
> On Mon, Dec 12, 2022 at 09:52:03AM -0800, Yonghong Song wrote:
>>
>>
>> On 12/12/22 9:02 AM, Benjamin Tissoires wrote:
>>> On Thu, Nov 3, 2022 at 4:58 PM Benjamin Tissoires
>>> <benjamin.tissoires@...hat.com> wrote:
>>>>
>>>> Kind of a hack, but works for now:
>>>>
>>>> Instead of listening for any close of eBPF program, we now
>>>> decrement the refcount when we insert it in our internal
>>>> map of fd progs.
>>>>
>>>> This is safe to do because:
>>>> - we listen to any call of destructor of programs
>>>> - when a program is being destroyed, we disable it by removing
>>>>     it from any RCU list used by any HID device (so it will never
>>>>     be called)
>>>> - we then trigger a job to cleanup the prog fd map, but we overwrite
>>>>     the removal of the elements to not do anything on the programs, just
>>>>     remove the allocated space
>>>>
>>>> This is better than previously because we can remove the map of known
>>>> programs and their usage count. We now rely on the refcount of
>>>> bpf, which has greater chances of being accurate.
>>>>
>>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
>>>>
>>>> ---
>>>
>>> So... I am a little bit embarrassed, but it turns out that this hack
>>> is not safe enough.
>>>
>>> If I compile the kernel with LLVM=1, the function
>>> bpf_prog_put_deferred() is optimized in a weird way: if we are not in
>>> irq, the function is inlined into __bpf_prog_put(), but if we are, the
>>> function is still kept around as it is called in a scheduled work
>>> item.
>>>
>>> This is something I completely overlooked: I assume that if the
>>> function would be inlined, the HID entrypoint BPF preloaded object
>>> would not be able to bind, thus deactivating HID-BPF safely. But if a
>>> function can be both inlined and not inlined, then I have no
>>> guarantees that my cleanup call will be called. Meaning that a HID
>>> device might believe there is still a bpf function to call. And things
>>> will get messy, with kernel crashes and others.
>>
>> You should not rely fentry to a static function. This is unstable
>> as compiler could inline it if that static function is called
>> directly. You could attach to a global function if it is not
>> compiled with lto.
> 
> But now that the kernel does support LTO, how can you be sure this will
> always work properly?  The code author does not know if LTO will kick in
> and optimize this away or not, that's the linker's job.

Ya, that is right. So for in-kernel bpf programs, attaching to global
functions are not safe either. For other not-in-kernel bpf programs, it
may not work but that is user's responsibility to adjust properly
(to different functions based on a particular build, etc.).

> 
> thanks,
> 
> greg k-h