On Fri, Jul 25, 2025 at 10:27 AM Casey Schaufler <ca...@schaufler-ca.com> wrote:
> On 7/24/2025 7:28 PM, Paul Moore wrote:
> > On Thu, Jul 24, 2025 at 11:39 AM Casey Schaufler <ca...@schaufler-ca.com>
> > wrote:
> >> On 7/21/2025 4:21 PM, Paul Moore wrote:
> >>> The LSM currently has a lot of code to maintain a list of the currently
> >>> active LSMs in a human readable string, with the only user being the
> >>> "/sys/kernel/security/lsm" code. Let's drop all of that code and
> >>> generate the string on first use and then cache it for subsequent use.
> >>>
> >>> Signed-off-by: Paul Moore <p...@paul-moore.com>
> >>> ---
> >>> include/linux/lsm_hooks.h | 1 -
> >>> security/inode.c | 59 +++++++++++++++++++++++++++++++++++++--
> >>> security/lsm_init.c | 49 --------------------------------
> >>> 3 files changed, 57 insertions(+), 52 deletions(-)
> > ..
> >
> >>> +/* NOTE: we never free the string below once it is set. */
> >>> +static DEFINE_SPINLOCK(lsm_read_lock);
> >>> +static char *lsm_read_str = NULL;
> >>> +static ssize_t lsm_read_len = 0;
> >>> +
> >>> static ssize_t lsm_read(struct file *filp, char __user *buf, size_t
> >>> count,
> >>> loff_t *ppos)
> >>> {
> >>> - return simple_read_from_buffer(buf, count, ppos, lsm_names,
> >>> - strlen(lsm_names));
> >>> + int i;
> >>> + char *str;
> >>> + ssize_t len;
> >>> +
> >>> +restart:
> >>> +
> >>> + rcu_read_lock();
> >>> + if (!lsm_read_str) {
> >>> + /* we need to generate the string and try again */
> >>> + rcu_read_unlock();
> >>> + goto generate_string;
> >>> + }
> >>> + len = simple_read_from_buffer(buf, count, ppos,
> >>> + rcu_dereference(lsm_read_str),
> >>> + lsm_read_len);
> >>> + rcu_read_unlock();
> >>> + return len;
> >>> +
> >>> +generate_string:
> >>> +
> >>> + for (i = 0; i < lsm_active_cnt; i++)
> >>> + /* the '+ 1' accounts for either a comma or a NUL */
> >>> + len += strlen(lsm_idlist[i]->name) + 1;
> >>> +
> >>> + str = kmalloc(len, GFP_KERNEL);
> >>> + if (!str)
> >>> + return -ENOMEM;
> >>> + str[0] = '\0';
> >>> +
> >>> + for (i = 0; i < lsm_active_cnt; i++) {
> >>> + if (i > 0)
> >>> + strcat(str, ",");
> >>> + strcat(str, lsm_idlist[i]->name);
> >>> + }
> >>> +
> >>> + spin_lock(&lsm_read_lock);
> >>> + if (lsm_read_str) {
> >>> + /* we raced and lost */
> >>> + spin_unlock(&lsm_read_lock);
> >>> + kfree(str);
> >>> + goto restart;
> >>> + }
> >>> + lsm_read_str = str;
> >>> + lsm_read_len = len;
> >> You're going to get a nul byte at the end of the string because
> >> you accounted for the ',' above, but there isn't one at the end
> >> of the string.
> > I'm not sure I understand your concern here, can you phrase it differently?
>
> "lockdown,capability,...,evm\0" You get the '\0' because you always expect
> a trailing ','. On the last element there is no ',' but the length is added
> as if there is.
>
> + lsm_read_len = len - 1;
>
> will fix the problem.
Ah, yes, gotcha. Thanks for catching this, the fix will be in the
next revision.
--
paul-moore.com
