Re: bpf: mmap_file LSM hook allows NULL pointer dereference

[Matt Bobrowski <mattbobrowski@google.com>]

On Wed, Dec 10, 2025 at 10:02:16AM +0000, Matt Bobrowski wrote:
> On Wed, Dec 03, 2025 at 10:23:43AM -0800, Alexei Starovoitov wrote:
> > On Wed, Dec 3, 2025 at 12:47 AM Matt Bobrowski <mattbobrowski@google.com> wrote:
> > >
> > > > We can play tricks with __weak. Like:
> > > >
> > > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > > index 7cb6e8d4282c..60d269a85bf1 100644
> > > > --- a/kernel/bpf/bpf_lsm.c
> > > > +++ b/kernel/bpf/bpf_lsm.c
> > > > @@ -21,7 +21,7 @@
> > > >   * function where a BPF program can be attached.
> > > >   */
> > > >  #define LSM_HOOK(RET, DEFAULT, NAME, ...)      \
> > > > -noinline RET bpf_lsm_##NAME(__VA_ARGS__)       \
> > > > +__weak noinline RET bpf_lsm_##NAME(__VA_ARGS__)        \
> > > >
> > > > diff kernel/bpf/bpf_lsm_proto.c
> > > >
> > > > +int bpf_lsm_mmap_file(struct file *file__nullable, unsigned long reqprot,
> > > > +                     unsigned long prot, unsigned long flags)
> > > > +{
> > > > +       return 0;
> > > > +}
> > > >
> > > > and above one with __nullable will be in vmlinux BTF.
> > > >
> > > > afaik __weak functions are not removed by linker when in non-LTO,
> > > > but it's still better than
> > > > +#define bpf_lsm_mmap_file bpf_lsm_mmap_file__original
> > > > No need to change bpf_lsm.h either.
> > >
> > > Annotating with a weak attribute would be quite nice, but the compiler
> > > will complain about the redefinition of the symbol
> > > bpf_lsm_mmap_file. To avoid this, we'd still need to rely on the
> > > rename and ignore dance by using the aforementioned define, which at
> > > that point would still result in both symbols being exposed in both
> > > BTF and the .text section.
> > 
> > Not quite. You missed this part in the above:
> > 
> > > > diff kernel/bpf/bpf_lsm_proto.c
> > 
> > it's a different file.
> 
> Yes, yes, this will work. However, as discussed, it's fundamentally
> reliant on a small "hack" which I've implemented within
> kernel/bpf/Makefile here [0] to workaround current pahole
> deduplication logic.
> 
> Andrii and Eduard,
> 
> I’d like your input on a pahole BTF generation issue which I've
> recently come across. In the series I just sent [0], I had to
> implement a workaround to force pahole to process bpf_lsm_proto.o
> before bpf_lsm.o.
> 
> This was necessary to ensure pahole generates BTF for the strong
> definition of bpf_lsm_mmap_file() (in bpf_lsm_proto.c) rather than the
> weak definition (in bpf_lsm.c). Without this forced ordering, pahole
> processed the weak definition first, resulting in a state array like
> this:
> 
> ```
> btf_encoder.func_states.array[N] = bpf_lsm_mmap_file (weak
> definition from bpf_lsm.o)
> 
> btf_encoder.func_states.array[N+1] = bpf_lsm_mmap_file (strong
> definition from bpf_lsm_proto.o)
> ```
> 
> Because the deduplication logic in btf_encoder__add_saved_funcs()
> folds duplicates (those determined by saved_functions_combine()) into
> the first occurrence, the resulting BTF was derived from the weak
> definition. This is incorrect, as the strong definition is the one
> actually linked into the final vmlinux image.
> 
> An obvious fix that immediately came to mind here was to essentially
> teach pahole about strong function prototype definitions, and prefer
> to emit BTF for those instead of any weak defined counterparts?

Thinking about this a little more. Perhaps whilst in
btf_encoder__add_saved_funcs() we should only emit BTF for any
duplicated function within a CU which happen to match the
corresponding entry within the backing ELF symtab? We can do this by
checking whether the virtual address stored within DW_AT_low_pc
matches that of what's stored in the st_value field for the
corresponding ELF symtab entry? For example, for bpf_lsm_mmap_file we
have:

Output from reading the vmlinux symbol table:
```
$ readelf -s <input> | grep bpf_lsm_mmap_file
165360: ffffffff8152f9b0    16 FUNC    GLOBAL DEFAULT    1 bpf_lsm_mmap_file
```
Output from reading the vmlinux DWARF debugging information:
```
<2a40982>   DW_AT_name        : (indirect string, offset: 0x1352ea): bpf_lsm_mmap_file
<2a40986>   DW_AT_decl_file   : 4
<2a40987>   DW_AT_decl_line   : 199
<2a40988>   DW_AT_decl_column : 1
<2a40989>   DW_AT_prototyped  : 1
<2a40989>   DW_AT_type        : <0x2a1b010>
<2a4098d>   DW_AT_low_pc      : 0xffffffff8152e260
<2a40995>   DW_AT_high_pc     : 0x10
<2a4099d>   DW_AT_frame_base  : 1 byte block: 9c    (DW_OP_call_frame_cfa)
<2a4099f>   DW_AT_call_all_calls: 1
<2a4099f>   DW_AT_sibling     : <0x2a409d8>
<2><2a409a3>: Abbrev Number: 10 (DW_TAG_formal_parameter)
<2a409a4>   DW_AT_name        : (indirect string, offset: 0x3623df): file
<2a409a8>   DW_AT_decl_file   : 4
<2a409a9>   DW_AT_decl_line   : 199
<2a409aa>   DW_AT_decl_column : 1
<2a409aa>   DW_AT_type        : <0x2a234ef>
<2a409ae>   DW_AT_location    : 1 byte block: 55    (DW_OP_reg5 (rdi))
<2><2a409b0>: Abbrev Number: 10 (DW_TAG_formal_parameter)
<2a409b1>   DW_AT_name        : (indirect string, offset: 0x23a09d): reqprot
<2a409b5>   DW_AT_decl_file   : 4
--
<2a60e0a>   DW_AT_name        : (indirect string, offset: 0x1352ea): bpf_lsm_mmap_file
<2a60e0e>   DW_AT_decl_file   : 1
<2a60e0f>   DW_AT_decl_line   : 15
<2a60e10>   DW_AT_decl_column : 5
<2a60e11>   DW_AT_prototyped  : 1
<2a60e11>   DW_AT_type        : <0x2a42713>
<2a60e15>   DW_AT_low_pc      : 0xffffffff8152f9b0
<2a60e1d>   DW_AT_high_pc     : 0x10
<2a60e25>   DW_AT_frame_base  : 1 byte block: 9c    (DW_OP_call_frame_cfa)
<2a60e27>   DW_AT_call_all_calls: 1
<2><2a60e27>: Abbrev Number: 82 (DW_TAG_formal_parameter)
<2a60e28>   DW_AT_name        : (indirect string, offset: 0x135ede): file__nullable
<2a60e2c>   DW_AT_decl_file   : 1
<2a60e2c>   DW_AT_decl_line   : 15
<2a60e2d>   DW_AT_decl_column : 36
<2a60e2e>   DW_AT_type        : <0x2a49f59>
<2a60e32>   DW_AT_location    : 1 byte block: 55    (DW_OP_reg5 (rdi))
```

> [0] https://lore.kernel.org/bpf/20251210090701.2753545-1-mattbobrowski@google.com/T/#me14d534fb559a349c46e094f18c63d477644d511