Duplicated module names
Lucas De Marchi writes: > Hi! > > CC'ing Rusty and mailing lists Thanks. > Rusty and ohers: it looks like both CONFIG_CRC32 and > CONFIG_CRYPTO_CRC32 can be compiled as module, and they generate > modules with the same name, crc32. Could that be fixed? Gah. Looks like it's been that way since at least 2014, too. I think we could rename it to crypto_crc32, but I don't think it's the only one. Marco, I think depmod should probably FAIL if two modules have the same name, which would at least find such problems. (BTW is there a nice way to figure out if a config var is a tristate? These are only problematic if both CONFIG_ are tristate.) Here's a hacky attempt to look for problems: rusty@rusty-Lemur:~/devel/kernel/linux (master)$ KCONFIGS=`find * -name 'Kconfig*'`; for m in `find [b-z]* -name 'Makefile*'`; do sed -n 's,obj-\$(CONFIG.*+= \([a-z0-9_-]\+\.o\)$,'$m' \1,p' <$m | sort -u; done | sort -k 2 | uniq -D -f 1 | while read m obj; do fgrep -w $obj $m /dev/null; done | while read LINE; do conf=`echo $LINE | sed 's/.*\$(CONFIG_\([A-Z0-9_]*\).*/\1/'`; if grep -C2 "^config $conf\$" $KCONFIGS | fgrep -q tristate; then echo $LINE; fi; done Here are the results (mildly filtered by me): drivers/gpu/drm/i2c/Makefile:obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o drivers/media/i2c/Makefile:obj-$(CONFIG_VIDEO_ADV7511) += adv7511.o drivers/media/platform/coda/Makefile:obj-$(CONFIG_VIDEO_CODA) += coda.o fs/coda/Makefile:obj-$(CONFIG_CODA_FS) += coda.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_CONNECTOR_ANALOG_TV) += connector-analog-tv.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_CONNECTOR_ANALOG_TV) += connector-analog-tv.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_CONNECTOR_DVI) += connector-dvi.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_CONNECTOR_HDMI) += connector-hdmi.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_CONNECTOR_HDMI) += connector-hdmi.o crypto/Makefile:obj-$(CONFIG_CRYPTO_CRC32) += crc32.o lib/Makefile:obj-$(CONFIG_CRC32) += crc32.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_ENCODER_OPA362) += encoder-opa362.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_ENCODER_OPA362) += encoder-opa362.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_ENCODER_TFP410) += encoder-tfp410.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_ENCODER_TPD12S015) += encoder-tpd12s015.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_DPI) += panel-dpi.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_DSI_CM) += panel-dsi-cm.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_DSI_CM) += panel-dsi-cm.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_LGPHILIPS_LB035Q02) += panel-lgphilips-lb035q02.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_LGPHILIPS_LB035Q02) += panel-lgphilips-lb035q02.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o drivers/gpu/drm/omapdrm/displays/Makefile:obj-$(CONFIG_DISPLAY_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o drivers/video/fbdev/omap2/omapfb/displays/Makefile:obj-$(CONFIG_FB_OMAP2_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o drivers/mtd/onenand/Makefile:obj-$(CONFIG_MTD_ONENAND_SAMSUNG) += samsung.o drivers/tty/serial/Makefile:obj-$(CONFIG_SERIAL_SAMSUNG) += samsung.o sound/soc/codecs/Makefile:obj-$(CONFIG_SND_SOC_AC97_CODEC) += snd-soc-ac97.o sound/soc/samsung/Makefile:obj-$(CONFIG_SND_SAMSUNG_AC97) += snd-soc-a
Re: crypto: api - Move module sig ifdef into accessor function
Herbert Xu writes:
> Currently we're hiding mod->sig_ok under an ifdef in open code.
> This patch adds a module_sig_ok accessor function and removes that
> ifdef.
>
> Cc: Rusty Russell
> Signed-off-by: Herbert Xu
Did you want me to take this via modules-next? Assuming not.
So:
Acked-by: Rusty Russell
Thanks,
Rusty.
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 8057c9f..c63836f 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -44,12 +44,9 @@ static inline int crypto_set_driver_name(struct crypto_alg
> *alg)
>
> static inline void crypto_check_module_sig(struct module *mod)
> {
> -#ifdef CONFIG_CRYPTO_FIPS
> - if (fips_enabled && mod && !mod->sig_ok)
> + if (fips_enabled && mod && !module_sig_ok(mod))
> panic("Module %s signature verification failed in FIPS mode\n",
> mod->name);
> -#endif
> - return;
> }
>
> static int crypto_check_alg(struct crypto_alg *alg)
> diff --git a/include/linux/module.h b/include/linux/module.h
> index c883b86..1e54360 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -655,4 +655,16 @@ static inline void module_bug_finalize(const Elf_Ehdr
> *hdr,
> static inline void module_bug_cleanup(struct module *mod) {}
> #endif /* CONFIG_GENERIC_BUG */
>
> +#ifdef CONFIG_MODULE_SIG
> +static inline bool module_sig_ok(struct module *module)
> +{
> + return module->sig_ok;
> +}
> +#else/* !CONFIG_MODULE_SIG */
> +static inline bool module_sig_ok(struct module *module)
> +{
> + return true;
> +}
> +#endif /* CONFIG_MODULE_SIG */
> +
> #endif /* _LINUX_MODULE_H */
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] hwrng: core - Use struct completion for cleanup_done
Herbert Xu writes:
> There is no point in doing a manual completion for cleanup_done
> when struct completion fits in perfectly.
>
> Signed-off-by: Herbert Xu
Indeed.
Acked-by: Rusty Russell
Thanks,
Rusty.
> ---
>
> drivers/char/hw_random/core.c | 12 +++-
> include/linux/hw_random.h |3 ++-
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 6ec4225..3dba2cf 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -60,7 +60,6 @@ static DEFINE_MUTEX(rng_mutex);
> static DEFINE_MUTEX(reading_mutex);
> static int data_avail;
> static u8 *rng_buffer, *rng_fillbuf;
> -static DECLARE_WAIT_QUEUE_HEAD(rng_done);
> static unsigned short current_quality;
> static unsigned short default_quality; /* = 0; default to "off" */
>
> @@ -100,10 +99,7 @@ static inline void cleanup_rng(struct kref *kref)
> if (rng->cleanup)
> rng->cleanup(rng);
>
> - /* cleanup_done should be updated after cleanup finishes */
> - smp_wmb();
> - rng->cleanup_done = true;
> - wake_up_all(&rng_done);
> + complete(&rng->cleanup_done);
> }
>
> static void set_current_rng(struct hwrng *rng)
> @@ -498,7 +494,7 @@ int hwrng_register(struct hwrng *rng)
> add_early_randomness(rng);
> }
>
> - rng->cleanup_done = false;
> + init_completion(&rng->cleanup_done);
>
> out_unlock:
> mutex_unlock(&rng_mutex);
> @@ -532,9 +528,7 @@ void hwrng_unregister(struct hwrng *rng)
> } else
> mutex_unlock(&rng_mutex);
>
> - /* Just in case rng is reading right now, wait. */
> - wait_event(rng_done, rng->cleanup_done &&
> -atomic_read(&rng->ref.refcount) == 0);
> + wait_for_completion(&rng->cleanup_done);
> }
> EXPORT_SYMBOL_GPL(hwrng_unregister);
>
> diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
> index 7832e50..eb7b414 100644
> --- a/include/linux/hw_random.h
> +++ b/include/linux/hw_random.h
> @@ -12,6 +12,7 @@
> #ifndef LINUX_HWRANDOM_H_
> #define LINUX_HWRANDOM_H_
>
> +#include
> #include
> #include
> #include
> @@ -46,7 +47,7 @@ struct hwrng {
> /* internal. */
> struct list_head list;
> struct kref ref;
> - bool cleanup_done;
> + struct completion cleanup_done;
> };
>
> /** Register a new Hardware Random Number Generator driver. */
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] hwrng: core - Fix current_rng init/cleanup race yet again
Herbert Xu writes:
> The kref solution is still buggy because we were only focusing
> on the register/unregister race. The same race affects the
> setting of current_rng through sysfs.
>
> This patch fixes it by using kref_get_unless_zero.
>
> Signed-off-by: Herbert Xu
This patch scares me a little!
I'll have to pull the tree to review it properly, but the theory was
that the reference count was counting users of the rng. Now I don't
know what it's counting:
> static inline int hwrng_init(struct hwrng *rng)
> {
> + if (kref_get_unless_zero(&rng->ref))
> + goto skip_init;
> +
> if (rng->init) {
> int ret;
OK, so this skip_init branch is triggered when the rng is being
shut down as it's no longer current_rng?
> +
> + kref_init(&rng->ref);
> + reinit_completion(&rng->cleanup_done);
> +
> +skip_init:
> add_early_randomness(rng);
Then we use it to add randomness?
>
> current_quality = rng->quality ? : default_quality;
> @@ -467,6 +474,9 @@ int hwrng_register(struct hwrng *rng)
> goto out_unlock;
> }
>
> + init_completion(&rng->cleanup_done);
> + complete(&rng->cleanup_done);
> +
This code smells very bad.
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] hwrng: core - Do not register device opportunistically
Herbert Xu writes:
> Currently we only register the device when a valid RNG is added.
> However the way it's done is buggy because we test whether there
> is a current RNG to determine whether we need to register. As
> the current RNG may be missing due to a reinitialisation error
> this can lead to a reregistration of the device.
>
> As the device already has to handle a NULL current RNG anyway,
> let's just register the device always and remove the complexity.
Does this break userspace by creating a device which will just return
-ENODEV on read? Sure, callers *should* handle it...
I far prefer this (simpler) model, though.
Thanks,
Rusty.
>
> Signed-off-by: Herbert Xu
> ---
>
> drivers/char/hw_random/core.c | 23 ---
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 42827fd..1d342f0 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -372,14 +372,14 @@ static DEVICE_ATTR(rng_available, S_IRUGO,
> NULL);
>
>
> -static void unregister_miscdev(void)
> +static void __exit unregister_miscdev(void)
> {
> device_remove_file(rng_miscdev.this_device, &dev_attr_rng_available);
> device_remove_file(rng_miscdev.this_device, &dev_attr_rng_current);
> misc_deregister(&rng_miscdev);
> }
>
> -static int register_miscdev(void)
> +static int __init register_miscdev(void)
> {
> int err;
>
> @@ -484,12 +484,6 @@ int hwrng_register(struct hwrng *rng)
> if (err)
> goto out_unlock;
> set_current_rng(rng);
> -
> - err = register_miscdev();
> - if (err) {
> - drop_current_rng();
> - goto out_unlock;
> - }
> }
> list_add_tail(&rng->list, &rng_list);
>
> @@ -530,7 +524,6 @@ void hwrng_unregister(struct hwrng *rng)
>
> if (list_empty(&rng_list)) {
> mutex_unlock(&rng_mutex);
> - unregister_miscdev();
> if (hwrng_fill)
> kthread_stop(hwrng_fill);
> } else
> @@ -540,16 +533,24 @@ void hwrng_unregister(struct hwrng *rng)
> }
> EXPORT_SYMBOL_GPL(hwrng_unregister);
>
> -static void __exit hwrng_exit(void)
> +static int __init hwrng_modinit(void)
> +{
> + return register_miscdev();
> +}
> +
> +static void __exit hwrng_modexit(void)
> {
> mutex_lock(&rng_mutex);
> BUG_ON(current_rng);
> kfree(rng_buffer);
> kfree(rng_fillbuf);
> mutex_unlock(&rng_mutex);
> +
> + unregister_miscdev();
> }
>
> -module_exit(hwrng_exit);
> +module_init(hwrng_modinit);
> +module_exit(hwrng_modexit);
>
> MODULE_DESCRIPTION("H/W Random Number Generator (RNG) driver");
> MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: Add soft module dependency to load HW accelerated crypto modules
Tim Chen writes: > On Fri, 2014-02-14 at 15:28 -0500, Neil Horman wrote: >> On Fri, Feb 14, 2014 at 11:14:37AM -0800, Tim Chen wrote: >> > We added the soft module dependency of various crypto algorithm's module >> > alias >> > to generic crypto algorithm's module. This loads hardware accelerated >> > modules and uses them when available. >> > >> This is great, but have any of the module load utilties been modified to >> recognize and handle soft dependencies in the modinfo section? Last I checked >> they hadn't. Do you plan to add that functionality? >> >> Neil >> > > Rusty, > > Do you know if the upstream modprobe has the fixes (or plan) to > recognize soft dependencies? I was under the impression that it > is the case. Marco CC'd... Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] modules: add support for soft module dependencies
Lucas De Marchi writes: > On Tue, Sep 17, 2013 at 11:10 PM, Rusty Russell wrote: >> Lucas De Marchi writes: >>> On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell >>> wrote: >>>> I'm happy to change this macro to create a modinfo line like >>>> "softdep:" >>> >>> how is that solving the issue that this macro can be used to designate >>> a mandatory or optional dependency >>> (https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is >>> mandatory we can very well let modprobe use that dependency during >>> module load >> >> I'm very close to sending Linus a revert commit at this point, since >> there's no consensus on what it's for. >> >> *Clearly* softdep shouldn't indicate a mandatory dependency. We already >> have a way (several) to make mandatory dependencies! >> >> And the "pre:" vs "post:" thing is just weird. If a module wants a post >> dependency, you can request_module() it from a workqueue. >> >>>> ie. tools like mkinitrd could pick it up and try to find a matching >>>> module, but depmod would ignore it. >>> >>> Some mkinitrd-like use whatever depmod/modprobe tells them it's >>> needed. So kmod still needs to know about it. >> >> It sounds like we should create a separate tool, which takes a list of >> modules and spits out the full pathname of all dependencies. *That* >> tool should include soft dependencies. >> >>>> It's really up to Lucas, since this affects him. >>> >>> IMO saying "this is an optional dependency and we can work without" >>> doesn't buy us much. Distros will end up putting the soft dep in >>> /etc/modules.d, kmod will always use them anyway and failing to load >>> the soft dep will fail the module load. I'd like to have no distro >>> files in /etc/modules.d in future. >> >> I assumed modprobe would handle soft dependencies in modules and try to >> pull them in, but *not* fail if they don't work. > > Iff the module doesn't *exist*. If it failed to load or failed for any > other reason then we will abort trying to load the other module. > However this is one thing we can change in modprobe to make it > consistent and more predictable. But we really need to reach a > consensus. It is a bit of a corner case, but there are other conceivable reasons why a module wouldn't load. The hardware it wants might not be present, or busy, or it conflicts with an existing module, or needs user configuration (eg. commandline params). It might be worth reporting, but I wouldn't stop loading on softdep fails. >> The previous way of doing this was: >> install foo modprobe foo_softdep 2>/dev/null; modprobe >> --ignore-install foo $CMDLINE_OPTS > > I just hope this is in no way an incentive for people using install commands > ;-) Agreed. install commands were implemented because I had no idea what people wanted to do in future. They definitely provide enough rope... Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] modules: add support for soft module dependencies
Lucas De Marchi writes: > On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell wrote: >> Lucas De Marchi writes: >>> On Wed, Jul 24, 2013 at 11:03 PM, Herbert Xu >>> wrote: >>>> On Thu, Jul 25, 2013 at 09:32:02AM +0930, Rusty Russell wrote: >>>>> Herbert Xu writes: >>>>> > Hi Rusty: >>>>> > >>>>> > I don't know why this patch never went into the kernel, even >>>>> > though the corresponding features have been added to modprobe >>>>> > in most if not all distros. >>>>> >>>>> Because Andreas never sent me the patch? This is the first I've *heard* >>>>> of this feature. Looks like it didn't hit lkml either. And what was >>>>> 2/2? >>>> >>>> 2/2 was the patch to actually use this in crc32c. >>>> >>>>> It's not how I would have done this: post-deps are more flexibly done at >>>>> runtime, because the module may have to do work to figure out what to >>>>> pull in. But since it already exists, I'll apply this patch: it doesn't >>>>> cost the kernel anything. >>> >>> But it did cause boot failures. The file modules.softdep file was >>> supposed to be informational until now. That's why depmod put a >>> comment saying to "copy on user's discretion to /etc/modules.d" >>> instead of parsing it directly. >> >> I'm happy to change this macro to create a modinfo line like >> "softdep:" > > how is that solving the issue that this macro can be used to designate > a mandatory or optional dependency > (https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is > mandatory we can very well let modprobe use that dependency during > module load I'm very close to sending Linus a revert commit at this point, since there's no consensus on what it's for. *Clearly* softdep shouldn't indicate a mandatory dependency. We already have a way (several) to make mandatory dependencies! And the "pre:" vs "post:" thing is just weird. If a module wants a post dependency, you can request_module() it from a workqueue. >> ie. tools like mkinitrd could pick it up and try to find a matching >> module, but depmod would ignore it. > > Some mkinitrd-like use whatever depmod/modprobe tells them it's > needed. So kmod still needs to know about it. It sounds like we should create a separate tool, which takes a list of modules and spits out the full pathname of all dependencies. *That* tool should include soft dependencies. >> It's really up to Lucas, since this affects him. > > IMO saying "this is an optional dependency and we can work without" > doesn't buy us much. Distros will end up putting the soft dep in > /etc/modules.d, kmod will always use them anyway and failing to load > the soft dep will fail the module load. I'd like to have no distro > files in /etc/modules.d in future. I assumed modprobe would handle soft dependencies in modules and try to pull them in, but *not* fail if they don't work. The previous way of doing this was: install foo modprobe foo_softdep 2>/dev/null; modprobe --ignore-install foo $CMDLINE_OPTS I agree this logic belongs in the kernel, we just have to figure out exactly how. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] modules: add support for soft module dependencies
Lucas De Marchi writes: > On Wed, Jul 24, 2013 at 11:03 PM, Herbert Xu > wrote: >> On Thu, Jul 25, 2013 at 09:32:02AM +0930, Rusty Russell wrote: >>> Herbert Xu writes: >>> > Hi Rusty: >>> > >>> > I don't know why this patch never went into the kernel, even >>> > though the corresponding features have been added to modprobe >>> > in most if not all distros. >>> >>> Because Andreas never sent me the patch? This is the first I've *heard* >>> of this feature. Looks like it didn't hit lkml either. And what was >>> 2/2? >> >> 2/2 was the patch to actually use this in crc32c. >> >>> It's not how I would have done this: post-deps are more flexibly done at >>> runtime, because the module may have to do work to figure out what to >>> pull in. But since it already exists, I'll apply this patch: it doesn't >>> cost the kernel anything. > > But it did cause boot failures. The file modules.softdep file was > supposed to be informational until now. That's why depmod put a > comment saying to "copy on user's discretion to /etc/modules.d" > instead of parsing it directly. I'm happy to change this macro to create a modinfo line like "softdep:" ie. tools like mkinitrd could pick it up and try to find a matching module, but depmod would ignore it. It's really up to Lucas, since this affects him. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] 3.11 boot failure caused by commit crypto
Herbert Xu writes:
> On Tue, Jul 30, 2013 at 03:26:50PM +0930, Rusty Russell wrote:
>>
>> Does this imply that you want me to push that to Linus now, and/or CC
>> stable? Was planning for *next* merge window...
>
> Next merge window is fine. However, it would be good to restore
> the new driver for that as well. So I see two ways to do this,
> either we push the crct10dif driver patches through your tree,
> on top of the MODULES_SOFTDEP patch, or I pull the MODULES_SOFTDEP
> patch into the crypto tree.
>
> What would you like to do?
You're welcome to it: it's v. unlikely to conflict with anything else.
Here it is, fresh from my pending-rebases branch, with my Signed-off-by.
Cheers,
Rusty.
From: Andreas Robinson
Subject: modules: add support for soft module dependencies
Additional and optional dependencies not found while building the kernel and
modules, can now be declared explicitly.
Signed-off-by: Andreas Robinson
Acked-by: Herbert Xu
Signed-off-by: Rusty Russell
---
include/linux/module.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/module.h b/include/linux/module.h
index 46f1ea0..504035f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -97,6 +97,11 @@ extern const struct gtype##_id __mod_##gtype##_table
\
/* For userspace: you can also call me... */
#define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)
+/* Soft module dependencies. See man modprobe.d for details.
+ * Example: MODULE_SOFTDEP("pre: module-foo module-bar post: module-baz")
+ */
+#define MODULE_SOFTDEP(_softdep) MODULE_INFO(softdep, _softdep)
+
/*
* The following license idents are currently accepted as indicating free
* software modules
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] 3.11 boot failure caused by commit crypto
Tim Chen writes: > On Tue, 2013-07-30 at 09:08 +1000, Herbert Xu wrote: >> On Mon, Jul 29, 2013 at 01:39:06PM -0700, Tim Chen wrote: >> > >> > Herbert, what are your thoughts on a proper fix to initrd issue for >> > crct10dif modules not getting included? Or can we >> > let the config option for the pclmul version to be either compiled >> > out or compiled in for now for people who do want to use this? >> >> With the MODULES_SOFTDEP patch that Rusty recently merged, all we >> need to do is to use that in lib/crct10dif module. Once that patch >> hits mainline I'll take care of the rest. > > Great. Thanks. > > Tim Does this imply that you want me to push that to Linus now, and/or CC stable? Was planning for *next* merge window... Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND/V2] crypto: Ignore validity dates of X.509 certificates at loading/parsing time
Alexander Holler writes: > Am 02.05.2013 16:09, schrieb Alexander Holler: >> I don't see any real use case where checking the validity dates of X.509 >> certificates at parsing time adds any security gain. In contrast, doing so >> makes MODSIGN unusable on systems without a RTC (or systems with a possible >> wrong date in a existing RTC, or systems where the RTC is read after the keys >> got loaded). >> >> If something really cares about the dates, it should check them at the time >> when the certificates are used, not when they are loaded and parsed. >> >> So just remove the validity check of the dates in the parser. >> >> Signed-off-by: Alexander Holler >> Cc: sta...@vger.kernel.org > > As it just happened to me again and I've recently posted some patches > which do make it possible to experience the problem on x86 systems too, > here is a reminder. > > To replay the problem (on x86 or any other arch), apply the 3 patches in > this series: > > https://lkml.org/lkml/2013/6/5/430 > > build a kernel with CONFIG_MODULE_SIG_FORCE=y and start that kernel with > hctosys=none as kernel command line parameter. > > This will disable the "persistent" clock (and any RTC), thus the kernel > will refuse to load modules because it doesn't has a valid time when > loading the certificate. > > Regards, > > Alexander Holler David? Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] modules
Linus Torvalds writes: > On Wed, Oct 10, 2012 at 2:57 AM, Rusty Russell wrote: >> >> >> module signing is the highlight, but it's an all-over David Howells frenzy... >> >> > > Hmm. What happened here? It *looks* from your pull request like you > had a tag, and you usually do, but there's no tag anywhere.. > > I've pulled and resolved the branch, and I'm going through it now, but > I'd like this verified before I push out if it all looks fine.. > > Linus Ah, I missed pushing the tag. I used to fabricate a git tree for you from my quilt series, and that script did the right thing. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] modules
The following changes since commit 925a6f0bf8bd122d5d2429af7f0ca0fecf4ae71f: Merge tag 'hwspinlock-3.6-fix' of git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock (2012-09-18 11:58:54 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next for you to fetch changes up to dbadc17683e6c673a69b236c0f041b931cc55c42: X.509: Fix indefinite length element skip error handling (2012-10-10 20:06:39 +1030) module signing is the highlight, but it's an all-over David Howells frenzy... David Howells (30): Make most arch asm/module.h files use asm-generic/module.h KEYS: Add payload preparsing opportunity prior to key instantiate or update MPILIB: Provide count_leading/trailing_zeros() based on arch functions KEYS: Document asymmetric key type KEYS: Implement asymmetric key type KEYS: Asymmetric key pluggable data parsers KEYS: Asymmetric public-key algorithm crypto key subtype KEYS: Provide signature verification with an asymmetric key MPILIB: Reinstate mpi_cmp[_ui]() and export for RSA signature verification RSA: Implement signature verification algorithm [PKCS#1 / RFC3447] RSA: Fix signature verification for shorter signatures X.509: Implement simple static OID registry X.509: Add utility functions to render OIDs as strings X.509: Add simple ASN.1 grammar compiler X.509: Add an ASN.1 decoder MPILIB: Provide a function to read raw data into an MPI X.509: Add a crypto key parser for binary (DER) X.509 certificates MODSIGN: Add FIPS policy MODSIGN: Provide gitignore and make clean rules for extra files MODSIGN: Provide Kconfig options MODSIGN: Automatically generate module signing keys if missing MODSIGN: Provide module signing public keys to the kernel MODSIGN: Implement module signature checking MODSIGN: Provide a script for generating a key ID from an X.509 cert MODSIGN: Sign modules during the build process MODSIGN: Use the same digest for the autogen key sig as for the module sig MODSIGN: Use utf8 strings in signer's name in autogenerated X.509 certs MODSIGN: Fix 32-bit overflow in X.509 certificate validity date checking X.509: Convert some printk calls to pr_devel X.509: Fix indefinite length element skip error handling Matthew Garrett (1): module: taint kernel when lve module is loaded Ralf Baechle (1): MIPS: Fix module.c build for 32 bit Randy Dunlap (1): asymmetric keys: fix printk format warning Rusty Russell (4): module: fix symbol waiting when module fails before init module: wait when loading a module which is currently initializing. module: signature checking hook MODSIGN: Make mrproper should remove generated files. .gitignore| 14 + Documentation/crypto/asymmetric-keys.txt | 312 ++ Documentation/kernel-parameters.txt |6 + Documentation/security/keys.txt | 50 +- Makefile |6 +- arch/Kconfig | 19 + arch/alpha/Kconfig|2 + arch/alpha/include/asm/module.h | 10 +- arch/arm/Kconfig |2 + arch/arm/include/asm/module.h |8 +- arch/avr32/Kconfig|2 + arch/avr32/include/asm/module.h |6 +- arch/blackfin/Kconfig |2 + arch/blackfin/include/asm/module.h|4 +- arch/c6x/Kconfig |1 + arch/c6x/include/asm/module.h | 12 +- arch/cris/Kconfig |1 + arch/cris/include/asm/Kbuild |2 + arch/cris/include/asm/module.h|9 - arch/frv/include/asm/module.h |8 +- arch/h8300/Kconfig|1 + arch/h8300/include/asm/Kbuild |2 + arch/h8300/include/asm/module.h | 11 - arch/hexagon/Kconfig |1 + arch/ia64/Kconfig |2 + arch/ia64/include/asm/module.h|6 +- arch/m32r/Kconfig |1 + arch/m32r/include/asm/Kbuild |2 + arch/m32r/include/asm/module.h| 10 - arch/m32r/kernel/module.c | 15 - arch/m68k/Kconfig |3 + arch/m68k/include/asm/module.h|6 +- arch/microblaze/Kconfig |1 + arch/mips/Kconfig |3 + arch/mips/include/asm/module.h| 10 +- arch/mips/kernel/Makefile |1 + arch/mips/kernel/module-rela.c| 145 +++ arch/
Re: [GIT PULL] Asymmetric keys and module signing
"Kasatkin, Dmitry" writes: > http://git.kernel.org/?p=linux/kernel/git/rusty/linux.git;a=commit;h=a15e196c5543d1d2d7f0cd70e62351aeb1f8b871 > > It breaks bisect.. > > CC kernel/module_signing.o > kernel/module_signing.c: In function ‘mod_verify_sig’: > kernel/module_signing.c:21:10: error: ‘ENOKEY’ undeclared (first use > in this function) > kernel/module_signing.c:21:10: note: each undeclared identifier is > reported only once for each function it appears in > kernel/module_signing.c:22:1: warning: control reaches end of non-void > function [-Wreturn-type] > make[1]: *** [kernel/module_signing.o] Error 1 > make: *** [kernel] Error 2 > make: *** Waiting for unfinished jobs Ah, my mistake; missing #include. I've fixed this which unfortunately meant a rebase. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next] asymmetric keys: fix printk format warning
Stephen Rothwell writes: > [Lets also cc Rusty who committed the patch ...] > > On Wed, 03 Oct 2012 16:04:46 -0700 Randy Dunlap wrote: >> >> From: Randy Dunlap >> >> Fix printk format warning in x509_cert_parser.c: >> >> crypto/asymmetric_keys/x509_cert_parser.c: In function 'x509_note_OID': >> crypto/asymmetric_keys/x509_cert_parser.c:113:3: warning: format '%zu' >> expects type 'size_t', but argument 2 has type 'long unsigned int' >> >> Builds cleanly on i386 and x86_64. Thanks, applied. Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes: > Rusty Russell wrote: > >> Right. I think we need to use different names for generated vs supplied >> files > > The problem with supplied files is people who do allyesconfig, allmodconfig > and randconfig just to test things finding that their builds break. The > kernel build magic is not really set up to handle external files like this. I > suppose make logic can be used to conditionally include stuff that might not > exist. > >> BTW, you missed a Signed-off-by: on your "MODSIGN: Use the same digest >> for the autogen key sig as for the module sig" patch. Please update. > > Done. > > I've also added a patch to convert the system clock to a struct tm and to > produce a struct tm within the ASN.1 decode and then compare those rather than > time_t values as a way to deal with the validity time overflow problem. We > may have to be able to handle certificates that we haven't generated that > stretch beyond 2038 (I wonder if we might find such in the UEFI key database > for example. OK, cherry-picked to replace my hack. It's in linux-next, and I will push in the next two days. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
Josh Boyer writes: > On Sat, Sep 29, 2012 at 08:13:25AM +0100, David Howells wrote: >> Rusty Russell wrote: >> >> > [2.808075] Loading module verification certificates >> > [2.809331] X.509: Cert 6e03943da0f3b015ba6ed7f5e0cac4fe48680994 has >> > expired >> > [2.810500] MODSIGN: Problem loading in-kernel X.509 certificate (-127) >> >> Hmmm... Other people have seen that. >> >> Ah! >> >> I wonder if the problem is that the certificate is valid for 100 years >> That might well cause an overflow on a 32-bit system. > > That does seem quite plausible. The comparisons are done with time_t, > which boils down to 'long' and 100 years in seconds would overflow > LONG_MAX. > >> Could you try changing the '36500' in kernel/Makefile to something shorter, >> like 365? > > I tried two values today. One close to LONG_MAX (24800 or ~68 years), > and 10 years (3650). The former still seemed to overflow, but > specifying a 10yr lifetime appears to have worked. That's because the timestamp is absolute, right? Indeed, that seems to be the limit here. Here's my solution (tested, and it breaks if you change 214730 to 214760 as expected): >From 2a4b91c2c29739191c6f7db9abee9296ae505c39 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 2 Oct 2012 12:55:06 +0930 Subject: [PATCH] MODSIGN: fix expiry of auto-generated certificates on 32-bit systems 100-year certificates make time_t wrap, resulting in: [2.835272] X.509: Cert a94f6776f3f5483b0764011d1fcc6c0298362e63 has expired [2.836346] MODSIGN: Problem loading in-kernel X.509 certificate (-127) Signed-off-by: Rusty Russell diff --git a/kernel/Makefile b/kernel/Makefile index e951adf..86336c9 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -168,6 +168,13 @@ endif ifeq ($(sign_key_with_hash),) $(error Could not determine digest type to use from kernel config) endif +ifeq ($(CONFIG_64BIT),y) +# 100 years is beyond my best-before date, anyway. +end_of_time_days=36500 +else +# Until 32-bit time_t wraps, with some slack. +end_of_time_days=$(shell expr \( 214730 - `date -u +%s` \) / 86400 ) +endif signing_key.priv signing_key.x509: x509.genkey @echo "###" @@ -180,7 +187,8 @@ signing_key.priv signing_key.x509: x509.genkey @echo "###" @echo "### rngd -r /dev/hwrandom" @echo "###" - openssl req -new -nodes -utf8 $(sign_key_with_hash) -days 36500 -batch \ + openssl req -new -nodes -utf8 $(sign_key_with_hash) \ + -days $(end_of_time_days) -batch \ -x509 -config x509.genkey \ -outform DER -out signing_key.x509 \ -keyout signing_key.priv -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] modsign: don't use bashism in sh scripts.
David Howells writes: > Rusty Russell wrote: > >> -source ./.config >> +. ./.config > > Does that make a difference? It does on Ubuntu, where /bin/sh => dash. "source" is a bashism. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes: > Rusty Russell wrote: > >> I noticed the Cert number didn't change with rebuilds: "distclean" >> didn't remove some files: >> >> $ git clean -f -f -x -d >> Removing extra_certificates >> Removing signing_key.priv >> Removing signing_key.x509 >> Removing signing_key.x509.keyid >> Removing signing_key.x509.signer >> Removing x509.genkey > > I'm not sure whether distclean should remove those, since they can be > externally supplied, or whether x509.genkey should have a Makefile dependency > on kernel/Makefile. > > I lean towards 'yes' when I'm altering kernel/Makefile trying to get things > right, and 'no' when I'm doing a make distclean to change my config or > changing kernel/Makefile for some unrelated reason. Right. I think we need to use different names for generated vs supplied files, then, because 'distclean' really must delete generated files. But it turns out you do try to clean these files, it just doesn't work. See below. I've applied this, as well, to my tree: git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next BTW, you missed a Signed-off-by: on your "MODSIGN: Use the same digest for the autogen key sig as for the module sig" patch. Please update. Cheers, Rusty. >From 8921e2ede91f93dcdbd08fa6613f8458de5f8afe Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 2 Oct 2012 14:35:24 +0930 Subject: [PATCH] MODSIGN: Make mrproper should remove generated files. It doesn't, because the clean targets don't include kernel/Makefile, and because two files were missing from the list. Signed-off-by: Rusty Russell diff --git a/Makefile b/Makefile index 1c88ec3..e70ebfe 100644 --- a/Makefile +++ b/Makefile @@ -995,7 +995,10 @@ MRPROPER_DIRS += include/config usr/include include/generated \ arch/*/include/generated MRPROPER_FILES += .config .config.old .version .old_version \ include/linux/version.h \ - Module.symvers tags TAGS cscope* GPATH GTAGS GRTAGS GSYMS + Module.symvers tags TAGS cscope* GPATH GTAGS GRTAGS GSYMS \ + signing_key.priv signing_key.x509 x509.genkey \ + extra_certificates signing_key.x509.keyid \ + signing_key.x509.signer # clean - Delete most, but leave enough to build external modules # diff --git a/kernel/Makefile b/kernel/Makefile index 86336c9..bb94783 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -216,4 +216,3 @@ x509.genkey: @echo >>x509.genkey "subjectKeyIdentifier=hash" @echo >>x509.genkey "authorityKeyIdentifier=keyid" endif -CLEAN_FILES += signing_key.priv signing_key.x509 x509.genkey extra_certificates -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes:
> Rusty Russell wrote:
>
>> And after those three fixes, I still get all fail:
>>
>> [3.361036] Request for unknown module key 'Magrathea: Glacier signing
>> key: 6
>> e03943da0f3b015ba6ed7f5e0cac4fe48680994' err -11
>
> Can you look back further in your kernel output, see if you can spot the bit
> where it's trying to load the keys. Look for things from modsign_pubkey.c:
>
> pr_notice("Loading module verification certificates\n");
> ...
> pr_err("MODSIGN: Problem loading in-kernel X.509
> certificate (%ld)\n",
> PTR_ERR(key));
> else
> pr_notice("MODSIGN: Loaded cert '%s'\n",
> key_ref_to_ptr(key)->description);
>
>> CONFIG_CRYPTO_SHA1=m
>
> Hmmm... I suspect it's that. We need a hash to verify the key's own
> signature too - and if you're using the key my autogen patch created for you,
> I think that would be SHA1, so that must be built in too.
Right, I chose SHA-512 because everyone knows it's 512 times more secure
than SHA-1.
I cherry-picked those two patches, and now I see:
[2.808075] Loading module verification certificates
[2.809331] X.509: Cert 6e03943da0f3b015ba6ed7f5e0cac4fe48680994 has expired
[2.810500] MODSIGN: Problem loading in-kernel X.509 certificate (-127)
I noticed the Cert number didn't change with rebuilds: "distclean"
didn't remove some files:
$ git clean -f -f -x -d
Removing extra_certificates
Removing signing_key.priv
Removing signing_key.x509
Removing signing_key.x509.keyid
Removing signing_key.x509.signer
Removing x509.genkey
Removing them didn't fix it either, but at least I got a new certificate.
This is x86-32 BTW. I've put the complete, built tree (minus .git dir)
up at http://ozlabs.org/~rusty/linux-for-dhowells.tar.xz
Here's how I run it:
kvm -nographic -m 256 -net user,restrict=off -net nic,model=virtio -drive
file=$QEMUIMAGE,index=0,media=disk,if=virtio -drive
file=$QEMUIMAGEB,index=1,media=disk,if=virtio -kernel arch/x86/boot/bzImage
-append "ro root=/dev/vda1 console=ttyS0 $*"
Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
Mimi Zohar writes: > On Wed, 2012-09-26 at 13:16 +0930, Rusty Russell wrote: >> David Howells writes: >> > The module signing patches provide: >> > >> > - Some fixes to Rusty's patch. Also an additional patch to extend the >> > policy >> >handling for modules signed with an unknown key and to handle FIPS mode. >> >> Ok, I merged some of this (after our previous accidentally-off-list >> discussion). > > Rusty, have you pushed this branch out yet? > > thanks, > > Mimi git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next (That's *not* where linux-next currently pulls from) Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] modules: don't call eu-strip if it doesn't exist.
Signed-off-by: Rusty Russell diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 90b1bb1..2a4d1a1 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -165,11 +165,13 @@ endif # We strip the module as best we can - note that using both strip and eu-strip # results in a smaller module than using either alone. +EU_STRIP = $(shell which eu-strip || echo true) + quiet_cmd_sign_ko_stripped_ko_unsigned = STRIP [M] $@ cmd_sign_ko_stripped_ko_unsigned = \ cp $< $@ && \ strip -x -g $@ && \ - eu-strip $@ + $(EU_STRIP) $@ ifeq ($(SIGN_MODULES),1) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] modsign: don't use bashism in sh scripts.
Signed-off-by: Rusty Russell diff --git a/scripts/sign-file b/scripts/sign-file index 1a472bb..e58e34e 100644 --- a/scripts/sign-file +++ b/scripts/sign-file @@ -10,7 +10,7 @@ scripts=`dirname $0` CONFIG_MODULE_SIG_SHA512=y if [ -r .config ] then -source ./.config +. ./.config fi key="$1" -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes: > Hi Rusty, > > Could you pull my tree? > > David > --- > > The following changes since commit eeea3ac912207dcf759b95b2b4c36f96bce583bf: > > Merge tag 'fixes-for-linus' of > git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc (2012-09-06 > 10:23:58 -0700) > > are available in the git repository at: > > > git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-modsign.git > modsign-post-KS > > for you to fetch changes up to 15765081423824e1ccc329264ae13f5ea87f3a85: > > MODSIGN: Sign modules during the build process (2012-09-26 10:11:06 +0100) warning: (MODULE_SIG) selects ASYMMETRIC_KEY_TYPE which has unmet direct dependencies (CRYPTO && KEYS) Ah, I see: + select CONFIG_KEYS + select CONFIG_CRYPTO I fixed this commit, rather than go around again. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes: > Hi Rusty, > > Could you pull my tree? And after those three fixes, I still get all fail: [3.361036] Request for unknown module key 'Magrathea: Glacier signing key: 6 e03943da0f3b015ba6ed7f5e0cac4fe48680994' err -11 rusty@rusty-x201:~/devel/kernel/linux (tmp-merge)$ grep '^CONFIG.*\(CRYPTO\|KEY\|MODULE\)' .config CONFIG_MODULES_USE_ELF_REL=y CONFIG_MODULES=y CONFIG_MODULE_UNLOAD=y CONFIG_MODULE_FORCE_UNLOAD=y CONFIG_MODULE_SIG=y CONFIG_MODULE_SIG_SHA512=y CONFIG_NET_KEY=m CONFIG_INPUT_KEYBOARD=y CONFIG_KEYBOARD_ATKBD=y CONFIG_HID_EZKEY=y CONFIG_THINKPAD_ACPI_HOTKEY_POLL=y CONFIG_KEYS=y CONFIG_CRYPTO=y CONFIG_CRYPTO_ALGAPI=y CONFIG_CRYPTO_ALGAPI2=y CONFIG_CRYPTO_AEAD2=y CONFIG_CRYPTO_BLKCIPHER=m CONFIG_CRYPTO_BLKCIPHER2=y CONFIG_CRYPTO_HASH=y CONFIG_CRYPTO_HASH2=y CONFIG_CRYPTO_RNG=m CONFIG_CRYPTO_RNG2=y CONFIG_CRYPTO_PCOMP2=y CONFIG_CRYPTO_MANAGER=m CONFIG_CRYPTO_MANAGER2=y CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y CONFIG_CRYPTO_NULL=m CONFIG_CRYPTO_WORKQUEUE=y CONFIG_CRYPTO_CBC=m CONFIG_CRYPTO_ECB=m CONFIG_CRYPTO_PCBC=m CONFIG_CRYPTO_HMAC=m CONFIG_CRYPTO_CRC32C=y CONFIG_CRYPTO_CRC32C_INTEL=m CONFIG_CRYPTO_MD4=m CONFIG_CRYPTO_MD5=y CONFIG_CRYPTO_MICHAEL_MIC=m CONFIG_CRYPTO_SHA1=m CONFIG_CRYPTO_SHA256=m CONFIG_CRYPTO_SHA512=y CONFIG_CRYPTO_TGR192=m CONFIG_CRYPTO_WP512=m CONFIG_CRYPTO_AES=m CONFIG_CRYPTO_ANUBIS=m CONFIG_CRYPTO_ARC4=m CONFIG_CRYPTO_BLOWFISH=m CONFIG_CRYPTO_BLOWFISH_COMMON=m CONFIG_CRYPTO_CAST5=m CONFIG_CRYPTO_CAST6=m CONFIG_CRYPTO_DES=m CONFIG_CRYPTO_KHAZAD=m CONFIG_CRYPTO_SERPENT=m CONFIG_CRYPTO_TEA=m CONFIG_CRYPTO_TWOFISH=m CONFIG_CRYPTO_TWOFISH_COMMON=m CONFIG_CRYPTO_DEFLATE=m CONFIG_CRYPTO_ANSI_CPRNG=m CONFIG_ASYMMETRIC_KEY_TYPE=y CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y CONFIG_PUBLIC_KEY_ALGO_RSA=y Clues welcome... Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes: > Rusty Russell wrote: > >> We do a very simple search for a particular string appended to the module >> (which is cache-hot and about to be SHA'd anyway). There's both a config >> option and a boot parameter which control whether we accept (and taint) or >> fail with unsigned modules. > > I've adjusted your patch description to this: > > We do a very simple search for a particular string appended to the module > (which is cache-hot and about to be SHA'd anyway). There's both a config > option and a boot parameter which control whether we accept or fail with > unsigned modules and modules that are signed with an unknown key. > > If module signing is enabled, the kernel will be tainted if a module is > accepted that is unsigned or has a signature for which we don't have the > key. > > I think it's worth mentioning the policy for unknown keys and worth making > clear under what circumstances we mean the kernel to be tainted. Great! I checked your Kconfig help, too, which is states it clearly: config MODULE_SIG_FORCE bool "Require modules to be validly signed" depends on MODULE_SIG help Reject unsigned modules or signed modules for which we don't have a key. Without this, such modules will simply taint the kernel. Which is really nice, since the kernel Kconfig help messages tend to suck. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Asymmetric keys and module signing
David Howells writes:
> The module signing patches provide:
>
> - Some fixes to Rusty's patch. Also an additional patch to extend the policy
>handling for modules signed with an unknown key and to handle FIPS mode.
Ok, I merged some of this (after our previous accidentally-off-list
discussion).
You previously wrote:
> You can't compare them that easily. One has a FIPS-mode panic and the other
> doesn't. Do we want to panic if we reject an unsigned module in enforcing
> mode when we're in FIPS mode?
It's a line ball, but I think consistency wins. Not a validly signed
module => panic.
The code becomes pretty straightforward then:
if (!err) {
info->sig_ok = true;
return 0;
}
if (fips_enabled)
panic("Module verification failed with error %d in FIPS mode\n",
err);
if (err == -ENOKEY && !sig_enforce)
err = 0;
return err;
In preparation, I've changed that below (and also, fixed up the -ENOKEY
which I said I'd do, and didn't).
Thanks,
Rusty.
PS. Agree with Kconfig options move, but I'll do that in separate patch.
From: Rusty Russell
Subject: module: signature checking hook
We do a very simple search for a particular string appended to the module
(which is cache-hot and about to be SHA'd anyway). There's both a config
option and a boot parameter which control whether we accept (and taint) or
fail with unsigned modules.
(Useful feedback and tweaks by David Howells )
Signed-off-by: Rusty Russell
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1582,6 +1582,12 @@ bytes respectively. Such letter suffixes
log everything. Information is printed at KERN_DEBUG
so loglevel=8 may also need to be specified.
+ module.sig_enforce
+ [KNL] When CONFIG_MODULE_SIG is set, this means that
+ modules without (valid) signatures will fail to load.
+ Note that if CONFIG_MODULE_SIG_ENFORCE is set, that
+ is always true, so this option does nothing.
+
mousedev.tap_time=
[MOUSE] Maximum time between finger touching and
leaving touchpad surface for touch to be considered
diff --git a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,6 +21,9 @@
#include
#include
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -260,6 +263,11 @@ struct module
const unsigned long *unused_gpl_crcs;
#endif
+#ifdef CONFIG_MODULE_SIG
+ /* Signature was verified. */
+ bool sig_ok;
+#endif
+
/* symbols that will be GPL-only in the near future. */
const struct kernel_symbol *gpl_future_syms;
const unsigned long *gpl_future_crcs;
diff --git a/init/Kconfig b/init/Kconfig
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1585,6 +1585,20 @@ config MODULE_SRCVERSION_ALL
the version). With this option, such a "srcversion" field
will be created for all modules. If unsure, say N.
+config MODULE_SIG
+ bool "Module signature verification"
+ depends on MODULES
+ help
+ Check modules for valid signatures upon load: the signature
+ is simply appended to the module. For more information see
+ Documentation/module-signing.txt.
+
+config MODULE_SIG_FORCE
+ bool "Require modules to be validly signed"
+ depends on MODULE_SIG
+ help
+ Reject unsigned modules or signed modules for which we don't have a
+ key. Without this, such modules will simply taint the kernel.
endif # MODULES
config INIT_ALL_POSSIBLE
diff --git a/kernel/Makefile b/kernel/Makefile
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock
obj-$(CONFIG_PROVE_LOCKING) += spinlock.o
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
+obj-$(CONFIG_MODULE_SIG) += module_signing.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_KEXEC) += kexec.o
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
new file mode 100644
--- /dev/null
+++ b/kernel/module-internal.h
@@ -0,0 +1,13 @@
+/* Module internals
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Ge
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Thu, 15 Dec 2011 00:14:31 +, David Howells wrote: > Rusty Russell wrote: > > > > > We can have false positives, but at worst that make us report EINVAL > > > > (bad signature) instead of ENOENT (no signature). > > > > > > EKEYREJECTED please; that way it's the same as RHEL does now. > > > > OK, sure (who knew that was there?). Oh yes, I read these, but I didn't appreciate that those errnos had existed for over 6 years. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Mon, 12 Dec 2011 16:11:27 +, David Howells wrote: > Rusty Russell wrote: > > > OK, then you need to generate stripped modules as part of the build, > > too. It's a bit of a pain, sure, but hardly a showstopper. > > They'd have to be maximally stripped so that mkinitrd doesn't do anything to > them, but you'd then get the debuginfo from them into the packaging if you're > on some distribution or other. And you also provide an option to not strip > them if whoever wants them unstripped. I was thinking we generate multiple modules; people will definitely want unstripped versions as well. > > A signature contains a magic marker > > I don't like this particularly - you can't guarantee that this won't be > generated by the assembler quite by accident. We don't care, in practice. It can't generate a valid key by accident. The only difference is that you might get "bad key" instead of "no key" if no sigature matches. But a validly parsable signature won't happen in practice. > You should find the end of the > ELF and work from there. It should be a simple matter of parsing the header > and the section table only, right? Then you can look at the file offset + > length of the last section in the file. Tried that first. It's more lines (which need to be carefully scrutinized)... > At that point, assuming this isn't > coincident with the actual end of the file, you can try parsing what's > thereafter as a signature. If it is actual PGP, then an RFC4880 parser should > recognise it as valid, and a signature packet should be seen. ...and you can still have padding. At which point, I realised that we might as well just scan the whole thing and be done. It's a bit cheeky, but I *know* it works, and can be verified by any reader without knowing anything about ELF or trusting the format at all. > > A signature contains a magic marker: it signs everything up to the > > magic marker (ie. just append them): > > SUM=`md5sum drivers/block/loop.ko | cut -d\ -f1`; echo "@Module > > signature:$SUM" >> drivers/block/loop.ko > > That's not a useful signature, but I suspect you're just showing this as an > example. Yeah, it was supposed to be a dumb example... > > We can have false positives, but at worst that make us report EINVAL > > (bad signature) instead of ENOENT (no signature). > > EKEYREJECTED please; that way it's the same as RHEL does now. OK, sure (who knew that was there?). > > Took me longer to figure out the damn crypto API > > You don't actually need to use that. The crypto API for the moment doesn't do > crytographic signature verification. Sure, I was working on mainline, which is why I chose md5 (plus using md5sum is easy). > returns -EBADMSG if none of its parsers recognise the signature, -ENOPKG if > the signature is recognised, but we can't handle it (for instance if it's an > unsupported hash algorithm), -ENOKEY if we recognise it, but there's no key > available or -EKEYREJECTED if we recognised it, found the matching key, but > the key couldn't be used to verify the signature for some reason. > > /* Call repeatedly to shovel data into the crypto hash */ > verify_sig_add_data(mod_sig, dataptr, datasize); > > /* Call to finalise and actually perform the verification */ > ret = verify_sig_end(mod_sig, sig, sig_size); > > or: > > /* Call to cancel the verification */ > verify_sig_cancel(mod_sig); > > This does all the work for you. That looks really nice, actually! Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Mon, 12 Dec 2011 01:21:40 +, David Howells wrote:
> Rusty Russell wrote:
>
> > I think you misunderstand, I'm talking about the modinfo command, not
> > the .modinfo section.
>
> Sorry, yes. But why do you need to enhance modinfo?
I was suggesting that you want it to print the signatures, or at least
indicate their existence. Maybe check them too, but that might be a bit
too heavy for modinfo.
> > But I need to know exactly what these version-dependent mangling of
> > modules is. Is it real? Is it more than strip? Is it so hard to fix
> > that it makes sense to add 450 lines of dense kernel code to allow
> > alteration of a module after signing?
>
> The strip program (as far as I know that's the only binutil that we need worry
> about) rearranges and reorders the section, symbol and relocation tables when
> it discards stuff, and different versions of strip have done it
> differently.
OK, then you need to generate stripped modules as part of the build,
too. It's a bit of a pain, sure, but hardly a showstopper.
> However, you said it should be fairly easy to jump over the ELF parcel to get
> to the signature. How do you plan on doing that?
> I presume you would just parse sufficient of the ELF to find the
> theoretical ELF EOF and then look there for a whole string of
> signatures
You could do that. But there's an easier way. Took me longer to figure
out the damn crypto API than actually write the module part :(
Subject: module: simple signature support.
A signature contains a magic marker: it signs everything up to the
magic marker (ie. just append them):
SUM=`md5sum drivers/block/loop.ko | cut -d\ -f1`; echo "@Module
signature:$SUM" >> drivers/block/loop.ko
We can have false positives, but at worst that make us report EINVAL
(bad signature) instead of ENOENT (no signature).
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2374,6 +2374,98 @@ free_hdr:
return err;
}
+/* CONFIG_MODULE_SIGN implies we don't trust modules: verify signature
+ * before we interpret (almost) anything. */
+#define MOD_SIGNATURE "@Module signature:"
+
+#include
+#include
+#include
+
+static int from_hex(char c)
+{
+ if (isdigit(c))
+ return c - '0';
+ if (isupper(c))
+ return c - 'A' + 10;
+ return c - 'a' + 10;
+}
+
+/* A signature signs everything before it. */
+static int try_signature(void *data, void *sig, unsigned long max_sig)
+{
+ unsigned long data_len = sig - data;
+
+ sig += strlen(MOD_SIGNATURE);
+ max_sig -= strlen(MOD_SIGNATURE);
+
+ /* Dummy: accept md5 as signature. */
+ {
+ struct crypto_api_blows {
+ struct shash_desc md5;
+ char morestuff[100];
+ } m;
+ u8 digest[MD5_DIGEST_SIZE], expected[MD5_DIGEST_SIZE];
+ char *s = sig;
+ int i;
+
+ /* Not a signature? */
+ if (max_sig < MD5_DIGEST_SIZE * 2) {
+ printk("Too close to end (%lu)\n", max_sig);
+ return -ENOENT;
+ }
+
+ for (i = 0; i < MD5_DIGEST_SIZE * 2; i += 2) {
+ /* Not a signature? */
+ if (!isxdigit(s[i]) || !isxdigit(s[i+1])) {
+ printk("Not hex digit (%c)\n", s[i]);
+ return -ENOENT;
+ }
+ digest[i/2] = (from_hex(s[i])<<4) | from_hex(s[i+1]);
+ }
+
+ m.md5.tfm = crypto_alloc_shash("md5", 0, 0);
+ if (IS_ERR(m.md5.tfm))
+ return PTR_ERR(m.md5.tfm);
+ m.md5.flags = 0;
+
+ crypto_shash_digest(&m.md5, data, data_len, expected);
+ crypto_free_shash(m.md5.tfm);
+
+ if (memcmp(digest, expected, sizeof(digest)) != 0) {
+ printk("Mismatch: given %02x%02x%02x...,"
+ " expect %02x%02x%02x...\n",
+ digest[0], digest[1], digest[2],
+ expected[0], expected[1], expected[2]);
+ return -EINVAL;
+ }
+ printk("Found valid signature!\n");
+ return 0;
+ }
+}
+
+/* -ENOENT if no signature found, -EINVAL if invalid, 0 if good. */
+static int find_and_check_signatures(struct load_info *info)
+{
+ void *p = info->hdr, *end = p + info->len;
+ const size_t sigsize = strlen(MOD_SIGNATURE);
+ int err = -ENOENT;
+
+ /* Poor man's memmem. len > sigsize */
+ whi
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Sat, 10 Dec 2011 14:08:34 +, David Howells wrote: > Rusty Russell wrote: > > > > > Sure, you now need to re-append that after stripping, but that's not the > > > > kernel's problem. > > > > > > You may also have to remove the signature before passing it to any > > > binutils tool lest it malfunction on the trailer > > > > Well, you're already on your own if you're using non-module-init-tools > > tools on modules. > > The distributions packaging tools and initramfs tools are things I have to > contend with, and others will have to contend with. If they're doing weird things to modules, yep, they'll break. They're not supposed to, but I know these things happen. But I'll need more than speculation, we'll need examples. > > (We'll want to enhance modinfo, at least, to show the signatures). > > Ummm... To show what exactly? And why? The modinfo has to go into the > signature hash. Further to find the modinfo, you have to parse the ELF - > which > brings us right back to how do you know you can trust it? I think you misunderstand, I'm talking about the modinfo command, not the .modinfo section. > > > I've found that rpmbuild and mkinitrd alter the module files at > > > various times, so you'd need a bunch of signatures, one for each (may > > > just be two, but I can't guarantee that). This means the kernel build > > > process needs to know what transformations are going to be applied to > > > a module - something that has changed occasionally within the > > > distribution I use and may vary between distributions (or even just > > > someone building for themselves). > > > > Yes, there may be more than stripped and unstripped. You may need to > > do fancy things. But now, adding a signature is so easy that it's not a > > real problem. And we can always have a hook, like: > > > > if VARIANTS=`make-module-variants $MOD`; then > > for m in $VARIANTS; do sign $m >> $MOD; rm $m; done > > fi > > That's not very practical. That spreads the what-do-we-need-to-calculate > question over a whole bunch of packages: the kernel, rpmbuild (if RPM-based), > mkinitramfs and maybe others. I presume you're thinking of trying all the > possible strip combinations and generating a signature for each? However, if > someone upgrades their binutils, but not their kernel, say, and then their > initramfs gets rebuilt for some reason, this may invalidate all their module > signatures. I'm assuming you either guarantee that strip produces identical results, or you generate all altered versions during the build (perhaps make-module-variants has a place it can stash them in the kernel tree where it gets rolled into your rpm). But I need to know exactly what these version-dependent mangling of modules is. Is it real? Is it more than strip? Is it so hard to fix that it makes sense to add 450 lines of dense kernel code to allow alteration of a module after signing? The answer may be "yes". But I need to know that before accepting your code. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Sat, 10 Dec 2011 10:37:23 -0800, Arjan van de Ven wrote: > > > > Yes, there may be more than stripped and unstripped. You may need to > > do fancy things. But now, adding a signature is so easy that it's > > not a real problem. And we can always have a hook, like: > > > > if VARIANTS=`make-module-variants $MOD`; then > > for m in $VARIANTS; do sign $m >> $MOD; rm $m; done > > fi > > but that requires you to keep the key around. No, that's the point. This was proposed as part of the kernel build. make-module-variants does one or more transforms on the module (eg. strip) and outputs the names of the results. If they're not reproducable, you have to keep them somewhere, yes, rather than remove them as here. > the most simple and common deployment of this is to generate a key, > build the public key into the kernel, sign the modules as you build the > kernel, and then destroy the key. > And THEN it gets deployed. Indeed. You have to demonstrate why the simplest way won't work, before putting 450 lines of extra checking into the kernel. Particularly since it's supposed to protect against malicious modules, so I need to *really* carefully review it. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Fri, 09 Dec 2011 18:43:26 +, David Howells wrote: > Rusty Russell wrote: > > > And adds a great deal of code in a supposedly security-sensitive path to > > achieve it. > > > > How about simply append a signature to the module? That'd be about 20 lines > > of code to carefully check the bounds of the module to figure out where the > > signature is. You could even allow multiple signatures, then have one for > > stripped, and one for non-stripped versions. > > A big chunk of the code is dealing with the cryptographic bits - and you need > those anyway - and if it's done right it can be shared with other things > (eCryptfs for example; maybe CIFS from what Steve French said) and auxiliary > keys can be stored in places other than the kernel (the TPM for > example). Sure, I was only commenting on the module parts, which are my responsibility. > > Sure, you now need to re-append that after stripping, but that's not the > > kernel's problem. > > You may also have to remove the signature before passing it to any > binutils tool lest it malfunction on the trailer Well, you're already on your own if you're using non-module-init-tools tools on modules. But objdump doesn't care about appended data. objcopy and strip will remove it, of course, but without complaining. > - and would you also > have to modify insmod and modprobe? I suspect they parse the ELF to > find out about parameters and things. No. modprobe needs to read .modinfo for --force, and modinfo does too. depmod is the worst: it actually looks through symtab and strtab, etc. And again, they're all fine. But even if they weren't, it wouldn't be an argument for putting all this in the kernel! (We'll want to enhance modinfo, at least, to show the signatures). > I've found that rpmbuild and mkinitrd alter the module files at > various times, so you'd need a bunch of signatures, one for each (may > just be two, but I can't guarantee that). This means the kernel build > process needs to know what transformations are going to be applied to > a module - something that has changed occasionally within the > distribution I use and may vary between distributions (or even just > someone building for themselves). Yes, there may be more than stripped and unstripped. You may need to do fancy things. But now, adding a signature is so easy that it's not a real problem. And we can always have a hook, like: if VARIANTS=`make-module-variants $MOD`; then for m in $VARIANTS; do sign $m >> $MOD; rm $m; done fi Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/21] MODSIGN: Apply signature checking to modules on module load [ver #3]
On Fri, 02 Dec 2011 18:46:51 +, David Howells wrote: > Signed modules may be safely stripped as the signature only covers those parts > of the module the kernel actually uses and any ELF metadata required to deal > with them. Any necessary ELF metadata that is affected by stripping is > canonicalised by the sig generator and the sig checker to hide strip effects. > > This permits the debuginfo to be detached from the module and placed > in another spot so that gdb can find it when referring to that module > without the need for multiple signed versions of the module. Such is > done by rpmbuild when producing RPMs. > > It also permits the module to be stripped as far as possible for when modules > are being reduced prior to being included in an initial ramdisk composition. And adds a great deal of code in a supposedly security-sensitive path to achieve it. How about simply append a signature to the module? That'd be about 20 lines of code to carefully check the bounds of the module to figure out where the signature is. You could even allow multiple signatures, then have one for stripped, and one for non-stripped versions. Sure, you now need to re-append that after stripping, but that's not the kernel's problem. Cheers, Rusty. PS. Yay for finding out about module patches via LWN! How would you get this in without my ack, FFS? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fixing gave up waiting for init of module libcrc32c.
On Thu, 1 Apr 2010 05:33:51 am Brandon Philips wrote: > On 09:36 Tue 30 Mar 2010, Rusty Russell wrote: > > The real fix here is to drop the lock, like Brandon suggested, but > > we need to do it more carefully: when we re-acquire the lock we need > > to re-lookup the symbol in case the module has vanished or changed. > > > > Brandon, I can't see how libcrc32c's module_init calls > > crypto_alloc_shash, but the problem is reproducible with simple > > example modules. Does it fix your problem? > > Did you see my email yesterday explaining how this came about? Does > that answer your question? Yep, thanks. > Reviewed the patch and it looks good and I tested it on the machine > and it works. A couple of trivial things inline below if you care. > > Signed-off-by: Brandon Philips > Cc: sta...@kernel.org > > Thanks Rusty. Thanks; already had the two caught by checkpatch; changing the other printk is not something I like to do in the same patch. Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fixing gave up waiting for init of module libcrc32c.
On Sat, 20 Mar 2010 10:59:59 pm Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 10:23:25PM -0700, David Miller wrote:
> >
> > I hear what you're saying Herbert, but thinking about this a bit I
> > really think we should make this situation work instead of fail.
>
> I think the initial report perhaps painted this in a slight
> different fashion than what it really is. The code that was
> looping in module.c is not trying to load libcrc32c, but rather
> it is trying to get a reference on the already-loaded libcrc32c
> module.
>
> AFAICS the only way to make it "work" would be to reload the
> module in question when we can't get a reference on it. But
> that would entail recursively loading a module during the process
> of loading another module.
>
> Rusty can chime in on whether this is doable.
Not really. The kernel code is pretty simple: if we can't find a symbol
we need, fail the load. To avoid nasty spurious races, we *do* wait if the
symbol we need is in a module which is still initializing. But we time out
after 30 seconds to avoid a module trainwreck.
The real fix here is to drop the lock, like Brandon suggested, but we need
to do it more carefully: when we re-acquire the lock we need to re-lookup
the symbol in case the module has vanished or changed.
Brandon, I can't see how libcrc32c's module_init calls crypto_alloc_shash,
but the problem is reproducible with simple example modules. Does it fix
your problem?
Tim: note that use_module() return value changes.
Thanks,
Rusty.
module: drop the lock while waiting for module to complete initialization.
This fixes "gave up waiting for init of module libcrc32c." which
happened at boot time due to multiple parallel module loads.
The problem was a deadlock: we wait for a module to finish
initializing, but we keep the module_lock mutex so it can't complete.
In particular, this could reasonably happen if libcrc32c does a
request_module() in its initialization routine.
So we change use_module() to return an errno rather than a bool, and if
it's -EBUSY we drop the lock and wait in the caller, then reaquire the
lock.
Reported-by: Brandon Philips
Signed-off-by: Rusty Russell
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -510,33 +510,25 @@ int use_module(struct module *a, struct
struct module_use *use;
int no_warn, err;
- if (b == NULL || already_uses(a, b)) return 1;
+ if (b == NULL || already_uses(a, b)) return 0;
/* If we're interrupted or time out, we fail. */
- if (wait_event_interruptible_timeout(
- module_wq, (err = strong_try_module_get(b)) != -EBUSY,
- 30 * HZ) <= 0) {
- printk("%s: gave up waiting for init of module %s.\n",
- a->name, b->name);
- return 0;
- }
-
- /* If strong_try_module_get() returned a different error, we fail. */
+ err = strong_try_module_get(b);
if (err)
- return 0;
+ return err;
DEBUGP("Allocating new usage for %s.\n", a->name);
use = kmalloc(sizeof(*use), GFP_ATOMIC);
if (!use) {
printk("%s: out of memory loading\n", a->name);
module_put(b);
- return 0;
+ return -ENOMEM;
}
use->module_which_uses = a;
list_add(&use->list, &b->modules_which_use_me);
no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
- return 1;
+ return 0;
}
EXPORT_SYMBOL_GPL(use_module);
@@ -823,7 +815,7 @@ static inline void module_unload_free(st
int use_module(struct module *a, struct module *b)
{
- return strong_try_module_get(b) == 0;
+ return strong_try_module_get(b);
}
EXPORT_SYMBOL_GPL(use_module);
@@ -994,17 +986,39 @@ static const struct kernel_symbol *resol
struct module *owner;
const struct kernel_symbol *sym;
const unsigned long *crc;
+ DEFINE_WAIT(wait);
+ int err;
+ long timeleft = 30 * HZ;
+again:
sym = find_symbol(name, &owner, &crc,
!(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)),
true);
- /* use_module can fail due to OOM,
- or module initialization or unloading */
- if (sym) {
- if (!check_version(sechdrs, versindex, name, mod, crc, owner)
- || !use_module(mod, owner))
- sym = NULL;
+ if (!sym)
+ return NULL;
+
+ if (!check_version(sechdrs, versindex, name, mod, crc, owner))
+ return NULL;
+
+ prepare_to_wait(&module_wq, &wait, TASK_INTERRUPTIBLE);
+ err = use_module(mod, owner);
+ if (likely(!err)
