Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Unsafe lockless call to i_size_write() causes seqcount corruption on 32-bit SMP systems. - [High] TOCTOU race in sysfs_btf_kernfs_read allows out-of-bounds kernel memory read on module reload. - [Medium] Reloading the btf_vmlinux_inline module permanently leaks the previously allocated BTF inline data. - [Medium] Unconditional dereference of btf_kobj->sd can lead to a boot-time kernel panic. --
commit 79f9afc658508547f1298b89f81b2e99fe704e3c Author: Alan Maguire <[email protected]> btf: Support CONFIG_DEBUG_INFO_BTF_INLINE=m Allow vmlinux BTF inline info to be delivered via a loadable module btf_vmlinux_inline.ko; this reduces the vmlinux binary size. A kernfs representation is created for vmlinux.inline which loads the module on demand. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 3e5890aed2dba..2ac1f1d396608 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c [ ... ] > @@ -8736,10 +8738,82 @@ enum { > }; > > #if IS_ENABLED(CONFIG_SYSFS) > +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF_INLINE) > +static struct bin_attribute *vmlinux_inline_attr; > +#endif [ ... ] > +static int sysfs_btf_kernfs_open(struct kernfs_open_file *of) > +{ > + struct bin_attribute *attr = of->kn->priv; > + size_t data_size; > + int err; [ ... ] > + if (of->file) { > + struct inode *inode = file_inode(of->file); > + > + if (inode) > + i_size_write(inode, data_size); [Severity: High] Does this code call i_size_write() locklessly? On 32-bit SMP systems, calling i_size_write() without holding inode_lock(inode) can cause seqcount corruption. This can lead to infinite loops in i_size_read() and a potential denial of service when unprivileged users concurrently open the sysfs file. > + } > + return 0; > +} > + > +static ssize_t sysfs_btf_kernfs_read(struct kernfs_open_file *of, char *buf, > + size_t bytes_requested, loff_t offset) > +{ > + struct bin_attribute *attr = of->kn->priv; > + void *data; > + size_t data_size; > + > + data_size = smp_load_acquire(&attr->size); > + if (offset >= data_size) > + return 0; > + if (offset + bytes_requested > data_size) > + bytes_requested = data_size - offset; > + data = READ_ONCE(attr->private); > + memcpy(buf, data + offset, bytes_requested); [Severity: High] Can this TOCTOU race lead to an out-of-bounds read? If a module reload triggers sysfs_btf_update() between the smp_load_acquire() of attr->size and the READ_ONCE() of attr->private, the new data pointer could be read while still using the old, potentially larger data_size. This might allow a local user to extract out-of-bounds kernel memory via the memcpy(). > + > + return bytes_requested; > +} [ ... ] > @@ -8755,7 +8829,18 @@ struct bin_attribute *sysfs_btf_add(const char *name, > void *data, size_t data_si > err = -ENOMEM; > goto err_free; > } > - err = sysfs_create_bin_file(btf_kobj, attr); > + if (data_size > 0) { > + err = sysfs_create_bin_file(btf_kobj, attr); > + } else { > + struct kernfs_node *node; > + > + node = __kernfs_create_file(btf_kobj->sd, attr->attr.name, [Severity: Medium] Is it possible for btf_kobj to be NULL here? If btf_kobj failed to allocate during btf_sysfs_init() and sysfs_btf_add() is called with data_size == 0 for vmlinux.inline, this unconditional dereference of btf_kobj->sd could lead to a boot-time kernel panic. > + attr->attr.mode, GLOBAL_ROOT_UID, > + GLOBAL_ROOT_GID, data_size, > + &sysfs_btf_kernfs_ops, attr, NULL, > NULL); > + if (IS_ERR(node)) > + err = PTR_ERR(node); > + } [ ... ] > @@ -8775,6 +8860,17 @@ struct bin_attribute *sysfs_btf_add(const char *name, > void *data, size_t data_si > } > #endif > > +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF_INLINE) > +static void sysfs_btf_update(struct bin_attribute *attr, void *data, size_t > data_size) > +{ > + if (!attr) > + return; > + WRITE_ONCE(attr->private, data); [Severity: Medium] Does this overwrite attr->private without freeing the previous allocation? When the btf_vmlinux_inline module is reloaded, this appears to permanently leak the previously allocated BTF inline data pointer. > + /* Publish data before its non-zero size makes it readable. */ > + smp_store_release(&attr->size, data_size); > +} > +#endif [ ... ] > @@ -8872,6 +8968,14 @@ static int btf_module_notify(struct notifier_block > *nb, unsigned long op, > err = 0; > goto out; > } > + if (strcmp(mod->name, "btf_vmlinux_inline") == 0) { > + if (vmlinux_inline_attr) > + sysfs_btf_update(vmlinux_inline_attr, > data, > + > mod->btf_inline_data_size); [Severity: Medium] Does this bypass assigning the data pointer to the module's tracking struct? Because btf_mod->btf_inline_data is left NULL in this branch, the newly allocated data won't be cleaned up when the module is unloaded, further contributing to the memory leak on module reload. > + else > + kvfree(data); > + break; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
