linux-kernel - Re: [PATCH] staging: Fix missing warning/taint on builtin code

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] [day] [month] [year] [list]

Date: Tue, 2 Jul 2024 14:11:47 -0300
From: Ágatha Isabelle Chris Moreira Guedes <code@...tha.dev>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Luis Chamberlain <mcgrof@...nel.org>, 
	Ágatha Isabelle Chris Moreira Guedes <patch-reply@...tha.dev>, Jookia <contact@...kia.org>
Subject: Re: [PATCH] staging: Fix missing warning/taint on builtin code

On Tue, Jul 02, 2024 at 09:50:49AM GMT, Uwe Kleine-König wrote:
> Hello Ágatha,
> 
> On Tue, Jul 02, 2024 at 02:44:31AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote:
> > ACKNOWLEDGEMENTS
> > Thanks for Jookia, heat and ukleinek for the important comments &
> > suggestions on this patch prior to submission.
> 
> FTR: That happend in the #kernelnewbies irc channel.

<3 <3 <3

> > diff --git a/include/linux/init.h b/include/linux/init.h
> > index 58cef4c2e59a..68c37600958f 100644
> > --- a/include/linux/init.h
> > +++ b/include/linux/init.h
> > @@ -397,4 +397,10 @@ void __init parse_early_options(char *cmdline);
> >  #define __exit_p(x) NULL
> >  #endif
> >  
> > +#ifdef CONFIG_STAGING
> > +#ifndef __ASSEMBLY__
> > +extern void staging_taint(const char *code_id, bool module);
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* CONFIG_STAGING */
> 
> You could drop the #ifdef for CONFIG_STAGING here. The obvious downside
> of this suggestion is that then you have a declaration of a function
> that isn't available depending on configuration. However the compiler
> doesn't care and the upside of not having CONFIG_STAGING in
> <linux/init.h> is that compile units that have nothing to do with
> CONFIG_STAGING but include <linux/init.h> won't get recompiled if you
> switch the setting.

Thanks a lot for the suggestion, it hasn't ocurred to me (despite the
intense task of recompiling it all over several times during
development LOL).

> 
> (OK, maybe a minor issue given that most drivers also include
> <linux/module.h> where the #ifdef cannot be dropped.)

For drivers it would be true, but with a very minimal config I am using,
the core code that doesn't include it would get some relative build time
improvement. Definetely worth, considering the machine I am using for
kernel development here (a core2quad cpu LOL).


> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 330ffb59efe5..ffe58f5d143b 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -76,6 +76,39 @@ extern struct module_attribute module_uevent;
> >  extern int init_module(void);
> >  extern void cleanup_module(void);
> >  
> > +#ifdef CONFIG_STAGING
> > +
> > +#define __lower_define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id)
> > +
> > +/**
> > + * __staging_define_initcall(fn,id) - staging initialization entry point
> > + * @fn: the function to run at kernel boot time
> > + * @id: the initcall level
> > + *
> > + * __staging_define_initcall() will ensure the drive's init function is always
> > + * called during initcalls for staging code by producing a wrapper function.
> > + * It applies if a module from the drivers/staging subtree is builtin to the
> > + * kernel. It reproduces the behavior in load_module() by tainting the kernel
> > + * and logging a warning about the code quality.
> > + */
> > +
> > +#define __staging_define_initcall(fn, id) \
> > +	static int __init __staging_wrapped_##fn(void) \
> > +	{ \
> > +		staging_taint(__FILE__, false); \
> > +		return fn(); \
> > +	} \
> > +__lower_define_initcall(__staging_wrapped_##fn, id)
> > +
> > +#ifdef STAGING_CODE
> > +
> > +#undef __define_initcall
> > +#define __define_initcall(fn, id) __staging_define_initcall(fn, id)
> 
> undefining macros makes the implemented logic hard to understand. While
> it allows to keep the touched code compact, in the long run it's more
> important that it's understandable.
> 
> So I suggest to modify the original definition of __define_initcall().

Yeah, I didn't like the idea of touching it at first, but it makes sense
at this time. Thanks for suggesting!

> 
> > +#endif /* STAGING_CODE */
> > +
> > +#endif /* CONFIG_STAGING */
> > +
> >  #ifndef MODULE
> >  /**
> >   * module_init() - driver initialization entry point
> 
> I like the idea and think the missing taint for built-in code is an
> oversight that went unnoticed for an astonishingly long time (since 2008
> if my quick research is right).
> 
> Maybe add
> 
> Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code")
> 
> to the commit log?!

I'll do, thanks a lot so far Uwe!

---
Sincerely,
Ágatha Isabelle
she/her
https://agatha.dev

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)