Re: [RFC PATCH 00/10] scalable symbol flags with __kflagstab

[Petr Pavlu <petr.pavlu@suse.com>]

On 9/4/25 1:28 AM, Sid Nayyar wrote:
> On Mon, Sep 1, 2025 at 1:27 PM Petr Pavlu <petr.pavlu@suse.com> wrote:
>> Merging __ksymtab and __ksymtab_gpl into a single section looks ok to
>> me, and similarly for __kcrctab and __kcrtab_gpl. The __ksymtab_gpl
>> support originally comes from commit 3344ea3ad4 ("[PATCH] MODULE_LICENSE
>> and EXPORT_SYMBOL_GPL support") [1], where it was named __gpl_ksymtab.
>> The commit doesn't mention why the implementation opts for using
>> a separate section, but I suspect it was designed this way to reduce
>> memory/disk usage.
>>
>> A question is whether the symbol flags should be stored in a new
>> __kflagstab section, instead of adding a flag member to the existing
>> __ksymtab. As far as I'm aware, no userspace tool (particularly kmod)
>> uses the __ksymtab data, so we are free to update its format.
>>
>> Note that I believe that __kcrctab/__kcrtab_gpl is a separate section
>> because the CRC data is available only if CONFIG_MODVERSIONS=y.
>>
>> Including the flags as part of __ksymtab would be obviously a simpler
>> schema. On the other hand, an entry in __ksymtab has in the worst case
>> a size of 24 bytes with an 8-byte alignment requirement. This means that
>> adding a flag to it would require additional 8 bytes per symbol.
> 
> Thanks for looking into the history of the _gpl split. We also noted
> that there were up to five separate arrays at one point.
> 
> We explored three approaches to this problem: using the existing
> __ksymtab, packing flags as bit-vectors, and the proposed
> __kflagstab. We ruled out the bit-vector approach due to its
> complexity, which would only save a few bits per symbol. The
> __ksymtab approach, while the simplest, was too wasteful of space.
> The __kflagstab seems like a good compromise, offering a slight
> increase in complexity over the __ksymtab method but requiring only
> one extra byte per symbol.

This sounds reasonable to me. Do you have any numbers on hand that would
show the impact of extending __ksymtab?

> 
>>>
>>> The motivation for this change comes from the Android kernel, which uses
>>> an additional symbol flag to restrict the use of certain exported
>>> symbols by unsigned modules, thereby enhancing kernel security. This
>>> __kflagstab can be implemented as a bitmap to efficiently manage which
>>> symbols are available for general use versus those restricted to signed
>>> modules only.
>>
>> I think it would be useful to explain in more detail how this protected
>> schema is used in practice and what problem it solves. Who is allowed to
>> provide these limited unsigned modules and if the concern is kernel
>> security, can't you enforce the use of only signed modules?
> 
> The Android Common Kernel source is compiled into what we call
> GKI (Generic Kernel Image), which consists of a kernel and a
> number of modules. We maintain a stable interface (based on CRCs and
> types) between the GKI components and vendor-specific modules
> (compiled by device manufacturers, e.g., for hardware-specific
> drivers) for the lifetime of a given GKI version.
> 
> This interface is intentionally restricted to the minimal set of
> symbols required by the union of all vendor modules; our partners
> declare their requirements in symbol lists. Any additions to these
> lists are reviewed to ensure kernel internals are not overly exposed.
> For example, we restrict drivers from having the ability to open and
> read arbitrary files. This ABI boundary also allows us to evolve
> internal kernel types that are not exposed to vendor modules, for
> example, when a security fix requires a type to change.
> 
> The mechanism we use for this is CONFIG_TRIM_UNUSED_KSYMS and
> CONFIG_UNUSED_KSYMS_WHITELIST. This results in a ksymtab
> containing two kinds of exported symbols: those explicitly required
> by vendors ("vendor-listed") and those only required by GKI modules
> ("GKI use only").
> 
> On top of this, we have implemented symbol import protection
> (covered in patches 9/10 and 10/10). This feature prevents vendor
> modules from using symbols that are not on the vendor-listed
> whitelist. It is built on top of CONFIG_MODULE_SIG. GKI modules are
> signed with a specific key, while vendor modules are unsigned and thus
> treated as untrusted. This distinction allows signed GKI modules to
> use any symbol in the ksymtab, while unsigned vendor modules can only
> access the declared subset. This provides a significant layer of
> defense and security against potentially exploitable vendor module
> code.

If I understand correctly, this is similar to the recently introduced
EXPORT_SYMBOL_FOR_MODULES() macro, but with a coarser boundary.

I think that if the goal is to control the kABI scope and limit the use
of certain symbols only to GKI modules, then having the protection
depend on whether the module is signed is somewhat odd. It doesn't give
me much confidence if vendor modules are unsigned in the Android
ecosystem. I would expect that you want to improve this in the long
term.

It would then make more sense to me if the protection was determined by
whether the module is in-tree (the "intree" flag in modinfo) or,
alternatively, if it is signed by a built-in trusted key. I feel this
way the feature could be potentially useful for other distributions that
care about the kABI scope and have ecosystems where vendor modules are
properly signed with some key. However, I'm not sure if this would still
work in your case.

> 
> Finally, we have implemented symbol export protection, which prevents
> a vendor module from impersonating a GKI module by exporting any of
> the same symbols. Note that this protection is currently specific to
> the Android kernel and is not included in this patch series.
> 
> We have several years of experience with older implementations of
> these protection mechanisms. The code in this series is a
> considerably cleaner and simpler version compared to what has been
> shipping in Android kernels since Android 14 (6.1 kernel).

I agree. I'm not aware of any other distribution that would immediately
need this feature, so it is good if the implementation is kept
reasonably straightforward and doesn't overly complicate the module
loader code.

-- 
Thanks,
Petr