Re: LZO irreversible output?
On Wednesday 03 February 2010, Nigel Cunningham wrote: > Hi all. > > (Not sent to LKML yesterday; no reply from linux-crypto yet, so resending). > > A while back now, I stopped supplying the LZF compression algorithm with > TuxOnIce and made LZO the default algorithm. Around the same time, we > started getting occasional errors when reading images; decompression > failures. > > I've finally managed to find the time to properly look at this, and have > managed to find a data page that LZO compresses, but seems to be unable > to decompress back to the original contents. I'm wondering whether this > is because I'm doing something wrong, or because there really is some > data the LZO (or the kernel implementation) can't do reversible > compression on. Well, FWIW, we have never had any problems with the userland LZO in s2disk, so if anything is wrong with LZO here, I guess it's the kernel code. Rafael -- 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 v4 23/27] x86_64: assembly, change all ENTRY+ENDPROC to SYM_FUNC_*
On Monday, October 2, 2017 11:12:42 AM CEST Jiri Slaby wrote: > These are all functions which are invoked from elsewhere, so we annotate > them as global using the new SYM_FUNC_START. And their ENDPROC's by > SYM_FUNC_END. > > And make sure ENTRY/ENDPROC is not defined on X86_64, given these were > the last users. > > Signed-off-by: Jiri Slaby > Cc: "H. Peter Anvin" > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: x...@kernel.org > Cc: Herbert Xu > Cc: "David S. Miller" > Cc: "Rafael J. Wysocki" > Cc: Len Brown > Cc: Pavel Machek > Cc: Matt Fleming > Cc: Ard Biesheuvel > Cc: Boris Ostrovsky > Cc: Juergen Gross > Cc: linux-crypto@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: xen-de...@lists.xenproject.org > --- > arch/x86/boot/compressed/efi_thunk_64.S| 4 +- > arch/x86/boot/compressed/head_64.S | 16 > arch/x86/crypto/aes-i586-asm_32.S | 8 ++-- > arch/x86/crypto/aes-x86_64-asm_64.S| 4 +- > arch/x86/crypto/aes_ctrby8_avx-x86_64.S| 12 +++--- > arch/x86/crypto/aesni-intel_asm.S | 44 > +++--- > arch/x86/crypto/aesni-intel_avx-x86_64.S | 24 ++-- > arch/x86/crypto/blowfish-x86_64-asm_64.S | 16 > arch/x86/crypto/camellia-aesni-avx-asm_64.S| 24 ++-- > arch/x86/crypto/camellia-aesni-avx2-asm_64.S | 24 ++-- > arch/x86/crypto/camellia-x86_64-asm_64.S | 16 > arch/x86/crypto/cast5-avx-x86_64-asm_64.S | 16 > arch/x86/crypto/cast6-avx-x86_64-asm_64.S | 24 ++-- > arch/x86/crypto/chacha20-avx2-x86_64.S | 4 +- > arch/x86/crypto/chacha20-ssse3-x86_64.S| 8 ++-- > arch/x86/crypto/crc32-pclmul_asm.S | 4 +- > arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 4 +- > arch/x86/crypto/crct10dif-pcl-asm_64.S | 4 +- > arch/x86/crypto/des3_ede-asm_64.S | 8 ++-- > arch/x86/crypto/ghash-clmulni-intel_asm.S | 8 ++-- > arch/x86/crypto/poly1305-avx2-x86_64.S | 4 +- > arch/x86/crypto/poly1305-sse2-x86_64.S | 8 ++-- > arch/x86/crypto/salsa20-x86_64-asm_64.S| 12 +++--- > arch/x86/crypto/serpent-avx-x86_64-asm_64.S| 24 ++-- > arch/x86/crypto/serpent-avx2-asm_64.S | 24 ++-- > arch/x86/crypto/serpent-sse2-x86_64-asm_64.S | 8 ++-- > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.S | 8 ++-- > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.S | 4 +- > arch/x86/crypto/sha1-mb/sha1_x8_avx2.S | 4 +- > arch/x86/crypto/sha1_avx2_x86_64_asm.S | 4 +- > arch/x86/crypto/sha1_ni_asm.S | 4 +- > arch/x86/crypto/sha1_ssse3_asm.S | 4 +- > arch/x86/crypto/sha256-avx-asm.S | 4 +- > arch/x86/crypto/sha256-avx2-asm.S | 4 +- > .../crypto/sha256-mb/sha256_mb_mgr_flush_avx2.S| 8 ++-- > .../crypto/sha256-mb/sha256_mb_mgr_submit_avx2.S | 4 +- > arch/x86/crypto/sha256-mb/sha256_x8_avx2.S | 4 +- > arch/x86/crypto/sha256-ssse3-asm.S | 4 +- > arch/x86/crypto/sha256_ni_asm.S| 4 +- > arch/x86/crypto/sha512-avx-asm.S | 4 +- > arch/x86/crypto/sha512-avx2-asm.S | 4 +- > .../crypto/sha512-mb/sha512_mb_mgr_flush_avx2.S| 8 ++-- > .../crypto/sha512-mb/sha512_mb_mgr_submit_avx2.S | 4 +- > arch/x86/crypto/sha512-mb/sha512_x4_avx2.S | 4 +- > arch/x86/crypto/sha512-ssse3-asm.S | 4 +- > arch/x86/crypto/twofish-avx-x86_64-asm_64.S| 24 ++-- > arch/x86/crypto/twofish-x86_64-asm_64-3way.S | 8 ++-- > arch/x86/crypto/twofish-x86_64-asm_64.S| 8 ++-- > arch/x86/entry/entry_64.S | 10 ++--- > arch/x86/entry/entry_64_compat.S | 8 ++-- > arch/x86/kernel/acpi/wakeup_64.S | 8 ++-- > arch/x86/kernel/head_64.S | 12 +++--- > arch/x86/lib/checksum_32.S | 8 ++-- > arch/x86/lib/clear_page_64.S | 12 +++--- > arch/x86/lib/cmpxchg16b_emu.S | 4 +- > arch/x86/lib/cmpxchg8b_emu.S | 4 +- > arch/x86/lib/copy_page_64.S| 4 +- > arch/x86/lib/copy_user_64.S| 16 > arch/x86/lib/csum-copy_64.S| 4 +- > arch/x86/lib/getuser.S | 16 > arch/x86/lib/hweight.S
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley wrote: > > On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote: > > On Sun, Nov 22, 2020 at 11:36 PM James Bottomley > > wrote: [cut] > > > > Maintainers routinely review 1-line trivial patches, not to mention > > internal API changes, etc. > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 Right. > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. > > > If some company does not want to pay for that, that's fine, but they > > don't get to be maintainers and claim `Supported`. > > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. > > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. Absolutely. This is just one of the factors involved, but a significant one IMV.
Re: [RFC PATCH 4/8] x86/power: Restore Key Locker internal key from the ACPI S3/4 sleep states
On Wed, Dec 16, 2020 at 6:47 PM Chang S. Bae wrote:
>
> When the system state switches to these sleep states, the internal key gets
> reset. Since this system transition is transparent to userspace, the
> internal key needs to be restored properly.
>
> Key Locker provides a mechanism to back up the internal key in non-volatile
> memory. The kernel requests a backup right after the key loaded at
> boot-time and copies it later when the system wakes up.
>
> The backup during the S5 sleep state is not trusted. It is overwritten by a
> new key at the next boot.
>
> On a system with the S3/4 states, enable the feature only when the backup
> mechanism is supported.
>
> Disable the feature when the copy fails (or the backup corrupts). The
> shutdown is considered too noisy. A new key is considerable only when
> threads can be synchronously suspended.
>
> Signed-off-by: Chang S. Bae
> Cc: x...@kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> ---
> arch/x86/include/asm/keylocker.h | 12
> arch/x86/kernel/cpu/common.c | 25 +++-
> arch/x86/kernel/keylocker.c | 51
> arch/x86/power/cpu.c | 34 +
> 4 files changed, 115 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/keylocker.h
> b/arch/x86/include/asm/keylocker.h
> index daf0734a4095..722574c305c2 100644
> --- a/arch/x86/include/asm/keylocker.h
> +++ b/arch/x86/include/asm/keylocker.h
> @@ -6,6 +6,7 @@
> #ifndef __ASSEMBLY__
>
> #include
> +#include
>
> #define KEYLOCKER_CPUID0x019
> #define KEYLOCKER_CPUID_EAX_SUPERVISOR BIT(0)
> @@ -25,5 +26,16 @@ void invalidate_keylocker_data(void);
> #define invalidate_keylocker_data() do { } while (0)
> #endif
>
> +static inline u64 read_keylocker_backup_status(void)
> +{
> + u64 status;
> +
> + rdmsrl(MSR_IA32_IWKEYBACKUP_STATUS, status);
> + return status;
> +}
> +
> +void backup_keylocker(void);
> +bool copy_keylocker(void);
> +
> #endif /*__ASSEMBLY__ */
> #endif /* _ASM_KEYLOCKER_H */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index d675075848bb..a446d5aff08f 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -463,24 +463,35 @@ __setup("nofsgsbase", x86_nofsgsbase_setup);
>
> static __always_inline void setup_keylocker(struct cpuinfo_x86 *c)
> {
> - bool keyloaded;
> -
> if (!cpu_feature_enabled(X86_FEATURE_KEYLOCKER))
> goto out;
>
> cr4_set_bits(X86_CR4_KEYLOCKER);
>
> if (c == &boot_cpu_data) {
> + bool keyloaded;
> +
> if (!check_keylocker_readiness())
> goto disable_keylocker;
>
> make_keylocker_data();
> - }
>
> - keyloaded = load_keylocker();
> - if (!keyloaded) {
> - pr_err_once("x86/keylocker: Failed to load internal key\n");
> - goto disable_keylocker;
> + keyloaded = load_keylocker();
> + if (!keyloaded) {
> + pr_err("x86/keylocker: Fail to load internal key\n");
> + goto disable_keylocker;
> + }
> +
> + backup_keylocker();
> + } else {
> + bool keycopied;
> +
> + /* NB: When system wakes up, this path recovers the internal
> key. */
> + keycopied = copy_keylocker();
> + if (!keycopied) {
> + pr_err_once("x86/keylocker: Fail to copy internal
> key\n");
> + goto disable_keylocker;
> + }
> }
>
> pr_info_once("x86/keylocker: Activated\n");
> diff --git a/arch/x86/kernel/keylocker.c b/arch/x86/kernel/keylocker.c
> index e455d806b80c..229875ac80d5 100644
> --- a/arch/x86/kernel/keylocker.c
> +++ b/arch/x86/kernel/keylocker.c
> @@ -5,11 +5,15 @@
> */
>
> #include
> +#include
> +#include
>
> #include
> #include
> #include
>
> +static bool keybackup_available;
> +
> bool check_keylocker_readiness(void)
> {
> u32 eax, ebx, ecx, edx;
> @@ -21,6 +25,14 @@ bool check_keylocker_readiness(void)
> return false;
> }
>
> + keybackup_available = (ebx & KEYLOCKER_CPUID_EBX_BACKUP);
> + /* Internal Key backup is essential with S3/4 states */
> + if (!keybackup_available &&
> + (acpi_sleep_state_supported(ACPI_STATE_S3) ||
> +acpi_sleep_state_supported(ACPI_STATE_S4))) {
> + pr_debug("x86/keylocker: no key backup support with possible
> S3/4\n");
> + return false;
> + }
> return true;
> }
>
> @@ -29,6 +41,7 @@ bool check_keylocker_readiness(void)
> #define LOADIWKEY_NUM_OPERANDS 3
>
> static struct key {
> + bool valid;
> struct reg_128_bit value[LOADIWKEY_NUM_OPERANDS];
> } keydata;
>
> @@ -38,11 +51,15 @@ void make_keylocker_data(void)
Re: Fix hibernation in FIPS mode?
On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
>
> Hi,
> MD5 was marked incompliant with FIPS in 2009:
> a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips
> mode")
> a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
>
> But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> due to the 2018 patch:
> 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest
> before hibernation")
>
> As a result, hibernation doesn't work when FIPS is on.
>
> Do you think if hibernation_e820_save() should be changed to use a
> FIPS-compliant algorithm like SHA-1?
I would say yes, it should.
> PS, currently it looks like FIPS mode is broken in the mainline:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
wrote:
>
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
> Signed-off-by: Chris von Recklinghausen
> ---
> arch/x86/power/hibernate.c | 31 +--
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..6a3f4e32e49c 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE 16
>
> struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> - u8 e820_digest[MD5_DIGEST_SIZE];
> + u8 e820_digest[CRC32_DIGEST_SIZE];
> };
No.
CRC32 was used here before and it was deemed insufficient.
Please find a different way to address this issue.
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
> /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
> *
> * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + * @buf: the crc32 result to be stored to
> */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static int get_e820_crc32(struct e820_table *table, void *buf)
> {
> struct crypto_shash *tfm;
> struct shash_desc *desc;
> int size;
> int ret = 0;
>
> - tfm = crypto_alloc_shash("md5", 0, 0);
> + tfm = crypto_alloc_shash("crc32", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void
> *buf)
>
> static int hibernation_e820_save(void *buf)
> {
> - return get_e820_md5(e820_table_firmware, buf);
> + return get_e820_crc32(e820_table_firmware, buf);
> }
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> int ret;
> - u8 result[MD5_DIGEST_SIZE];
> + u8 result[CRC32_DIGEST_SIZE];
>
> - memset(result, 0, MD5_DIGEST_SIZE);
> + memset(result, 0, CRC32_DIGEST_SIZE);
> /* If there is no digest in suspend kernel, let it go. */
> - if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> + if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> return false;
>
> - ret = get_e820_md5(e820_table_firmware, result);
> + ret = get_e820_crc32(e820_table_firmware, result);
> if (ret)
> return true;
>
> - return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> + return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
> }
> #else
> static int hibernation_e820_save(void *buf)
> @@ -134,7 +134,7 @@ static int hibernation_e820_save(void *buf)
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> - /* If md5 is not builtin for restore kernel, let it go. */
> + /* If crc32 is not builtin for restore kernel, let it go. */
> return false;
> }
> #endif
> @@ -160,6 +160,9 @@ int arch_hibernation_header_save(void *addr, unsigned int
> max_size)
> rdr->jump_address = (unsigned long)restore_registers;
> rdr->jump_address_phys = __pa_symbol(restore_registers);
>
> + /* crc32 digest size is 4 but digest buffer size is 16 so zero it all
> */
> + memset(rdr->e820_digest, 0, CRC32_DIGEST_SIZE);
> +
> /*
> * The restore code fixes up CR3 and CR4 in the following sequence:
> *
> --
> 2.18.1
>
Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 1, 2021 at 3:59 PM Ard Biesheuvel wrote:
>
> On Thu, 1 Apr 2021 at 15:34, Rafael J. Wysocki wrote:
> >
> > On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
> > wrote:
> > >
> > > Suspend fails on a system in fips mode because md5 is used for the e820
> > > integrity check and is not available. Use crc32 instead.
> > >
> > > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820
> > > memory map
> > >by md5 digest")
> > > Signed-off-by: Chris von Recklinghausen
> > > ---
> > > arch/x86/power/hibernate.c | 31 +--
> > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> > > index cd3914fc9f3d..6a3f4e32e49c 100644
> > > --- a/arch/x86/power/hibernate.c
> > > +++ b/arch/x86/power/hibernate.c
> > > @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> > > }
> > >
> > >
> > > -#define MD5_DIGEST_SIZE 16
> > > +#define CRC32_DIGEST_SIZE 16
> > >
> > > struct restore_data_record {
> > > unsigned long jump_address;
> > > unsigned long jump_address_phys;
> > > unsigned long cr3;
> > > unsigned long magic;
> > > - u8 e820_digest[MD5_DIGEST_SIZE];
> > > + u8 e820_digest[CRC32_DIGEST_SIZE];
> > > };
> >
> > No.
> >
> > CRC32 was used here before and it was deemed insufficient.
> >
>
> Why? The git commit log does not have an explanation of this.
IIRC there was an example of a memory map that would produce the same
CRC32 value as the original or something like that.
But that said this code is all about failing more gracefully, so I
guess it isn't a big deal if the failure is more graceful in fewer
cases ...
Re: Fix hibernation in FIPS mode?
On Thu, Apr 1, 2021 at 6:22 PM Simo Sorce wrote:
>
> On Thu, 2021-04-01 at 18:02 +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 1, 2021 at 3:54 PM Ard Biesheuvel wrote:
> > > On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki wrote:
> > > > On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel wrote:
> > > > > On Tue, 30 Mar 2021 at 21:56, Simo Sorce wrote:
> > > > > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce wrote:
> > > > > > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui
> > > > > > > > > wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged
> > > > > > > > > > fips_allowed in fips mode")
> > > > > > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips
> > > > > > > > > > mode")
> > > > > > > > > >
> > > > > > > > > > But hibernation_e820_save() is still using MD5, and fails
> > > > > > > > > > in FIPS mode
> > > > > > > > > > due to the 2018 patch:
> > > > > > > > > > 749fa17093ff ("PM / hibernate: Check the success of
> > > > > > > > > > generating md5 digest before hibernation")
> > > > > > > > > >
> > > > > > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > > > > > >
> > > > > > > > > > Do you think if hibernation_e820_save() should be changed
> > > > > > > > > > to use a
> > > > > > > > > > FIPS-compliant algorithm like SHA-1?
> > > > > > > > >
> > > > > > > > > I would say yes, it should.
> > > > > > > > >
> > > > > > > > > > PS, currently it looks like FIPS mode is broken in the
> > > > > > > > > > mainline:
> > > > > > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > > > > > >
> > > > > > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > > > > > constructions and only for specified uses. If you need to change
> > > > > > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > > > > > >
> > > > > > >
> > > > > > > What is the reason for using a [broken] cryptographic hash here?
> > > > > > > if
> > > > > > > this is just an integrity check, better use CRC32
> > > >
> > > > Not really.
> > > >
> > > > CRC32 is not really sufficient for integrity checking here AFAICS. It
> > > > might be made a fallback option if MD5 is not available, but making it
> > > > the default would be somewhat over the top IMO.
> > > >
> > > > > > If the integrity check is used exclusively to verify there were no
> > > > > > accidental changes and is not used as a security measure, by all
> > > > > > means
> > > > > > I agree that using crc32 is a better idea.
> > > > > >
> > > > >
> > > > > Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> > > > > this, it is only a best effort check which is simply omitted if md5
> > > > > happens to be unavailable, so there is definitely no need for crypto
> > > > > here.
> > > >
> > > > Yes, it is about integrity checking only. No, CRC32 is not equivalent
> > > > to MD5 in that respect AFAICS.
> > > >
> > >
> > > There are two possibilities:
> > > - we care about an adversary attempting to forge a collision, in which
> > > case you need a cryptographic hash which is not broken;
> > > - we only care about integrity, in which case crypto is overkill, and
> > > CRC32 is sufficient. (Note that the likelihood of an honest,
> > > inadv
Re: Fix hibernation in FIPS mode?
On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel wrote:
>
> On Tue, 30 Mar 2021 at 21:56, Simo Sorce wrote:
> >
> > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce wrote:
> > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui
> > > > > wrote:
> > > > > > Hi,
> > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed
> > > > > > in fips mode")
> > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> > > > > >
> > > > > > But hibernation_e820_save() is still using MD5, and fails in FIPS
> > > > > > mode
> > > > > > due to the 2018 patch:
> > > > > > 749fa17093ff ("PM / hibernate: Check the success of generating md5
> > > > > > digest before hibernation")
> > > > > >
> > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > >
> > > > > > Do you think if hibernation_e820_save() should be changed to use a
> > > > > > FIPS-compliant algorithm like SHA-1?
> > > > >
> > > > > I would say yes, it should.
> > > > >
> > > > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > >
> > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > constructions and only for specified uses. If you need to change
> > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > >
> > >
> > > What is the reason for using a [broken] cryptographic hash here? if
> > > this is just an integrity check, better use CRC32
Not really.
CRC32 is not really sufficient for integrity checking here AFAICS. It
might be made a fallback option if MD5 is not available, but making it
the default would be somewhat over the top IMO.
> > If the integrity check is used exclusively to verify there were no
> > accidental changes and is not used as a security measure, by all means
> > I agree that using crc32 is a better idea.
> >
>
> Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> this, it is only a best effort check which is simply omitted if md5
> happens to be unavailable, so there is definitely no need for crypto
> here.
Yes, it is about integrity checking only. No, CRC32 is not equivalent
to MD5 in that respect AFAICS.
Thanks!
Re: Fix hibernation in FIPS mode?
On Thu, Apr 1, 2021 at 3:54 PM Ard Biesheuvel wrote:
>
> On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki wrote:
> >
> > On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel wrote:
> > >
> > > On Tue, 30 Mar 2021 at 21:56, Simo Sorce wrote:
> > > >
> > > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce wrote:
> > > > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui
> > > > > > > wrote:
> > > > > > > > Hi,
> > > > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged
> > > > > > > > fips_allowed in fips mode")
> > > > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips
> > > > > > > > mode")
> > > > > > > >
> > > > > > > > But hibernation_e820_save() is still using MD5, and fails in
> > > > > > > > FIPS mode
> > > > > > > > due to the 2018 patch:
> > > > > > > > 749fa17093ff ("PM / hibernate: Check the success of generating
> > > > > > > > md5 digest before hibernation")
> > > > > > > >
> > > > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > > > >
> > > > > > > > Do you think if hibernation_e820_save() should be changed to
> > > > > > > > use a
> > > > > > > > FIPS-compliant algorithm like SHA-1?
> > > > > > >
> > > > > > > I would say yes, it should.
> > > > > > >
> > > > > > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > > > >
> > > > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > > > constructions and only for specified uses. If you need to change
> > > > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > > > >
> > > > >
> > > > > What is the reason for using a [broken] cryptographic hash here? if
> > > > > this is just an integrity check, better use CRC32
> >
> > Not really.
> >
> > CRC32 is not really sufficient for integrity checking here AFAICS. It
> > might be made a fallback option if MD5 is not available, but making it
> > the default would be somewhat over the top IMO.
> >
> > > > If the integrity check is used exclusively to verify there were no
> > > > accidental changes and is not used as a security measure, by all means
> > > > I agree that using crc32 is a better idea.
> > > >
> > >
> > > Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> > > this, it is only a best effort check which is simply omitted if md5
> > > happens to be unavailable, so there is definitely no need for crypto
> > > here.
> >
> > Yes, it is about integrity checking only. No, CRC32 is not equivalent
> > to MD5 in that respect AFAICS.
> >
>
> There are two possibilities:
> - we care about an adversary attempting to forge a collision, in which
> case you need a cryptographic hash which is not broken;
> - we only care about integrity, in which case crypto is overkill, and
> CRC32 is sufficient. (Note that the likelihood of an honest,
> inadvertent modification not being caught by CRC32 is 1 in 4 billion)
That depends on how you count.
Surely, there are modifications caught by MD5 that will not be caught by CRC32.
> MD5 does not meet either requirement, given that it is known to be
> broken, and overkill for simple integrity checks. MD5 should be phased
> out and removed, and moving this code onto the correct abstraction
> would be a reasonable step towards that goal.
This clearly is a matter of opinion.
I'm not religious about it though. If there is a general consensus
that CRC32 is sufficient for error detection in hibernation files,
then it can be used. So is there such a consensus and if so, can you
give me a pointer to some research that it is based on?
Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 1, 2021 at 3:34 PM Rafael J. Wysocki wrote:
>
> On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
> wrote:
> >
> > Suspend fails on a system in fips mode because md5 is used for the e820
> > integrity check and is not available. Use crc32 instead.
> >
> > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory
> > map
> >by md5 digest")
> > Signed-off-by: Chris von Recklinghausen
> > ---
> > arch/x86/power/hibernate.c | 31 +--
> > 1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> > index cd3914fc9f3d..6a3f4e32e49c 100644
> > --- a/arch/x86/power/hibernate.c
> > +++ b/arch/x86/power/hibernate.c
> > @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> > }
> >
> >
> > -#define MD5_DIGEST_SIZE 16
> > +#define CRC32_DIGEST_SIZE 16
> >
> > struct restore_data_record {
> > unsigned long jump_address;
> > unsigned long jump_address_phys;
> > unsigned long cr3;
> > unsigned long magic;
> > - u8 e820_digest[MD5_DIGEST_SIZE];
> > + u8 e820_digest[CRC32_DIGEST_SIZE];
> > };
>
> No.
>
> CRC32 was used here before and it was deemed insufficient.
>
> Please find a different way to address this issue.
Well, I guess I'm going to change my mind on this, so we can go with
this patch, but please bump up RESTORE_MAGIC too.
> > -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> > +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
> > /**
> > - * get_e820_md5 - calculate md5 according to given e820 table
> > + * get_e820_crc32 - calculate crc32 according to given e820 table
> > *
> > * @table: the e820 table to be calculated
> > - * @buf: the md5 result to be stored to
> > + * @buf: the crc32 result to be stored to
> > */
> > -static int get_e820_md5(struct e820_table *table, void *buf)
> > +static int get_e820_crc32(struct e820_table *table, void *buf)
> > {
> > struct crypto_shash *tfm;
> > struct shash_desc *desc;
> > int size;
> > int ret = 0;
> >
> > - tfm = crypto_alloc_shash("md5", 0, 0);
> > + tfm = crypto_alloc_shash("crc32", 0, 0);
> > if (IS_ERR(tfm))
> > return -ENOMEM;
> >
> > @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table,
> > void *buf)
> >
> > static int hibernation_e820_save(void *buf)
> > {
> > - return get_e820_md5(e820_table_firmware, buf);
> > + return get_e820_crc32(e820_table_firmware, buf);
> > }
> >
> > static bool hibernation_e820_mismatch(void *buf)
> > {
> > int ret;
> > - u8 result[MD5_DIGEST_SIZE];
> > + u8 result[CRC32_DIGEST_SIZE];
> >
> > - memset(result, 0, MD5_DIGEST_SIZE);
> > + memset(result, 0, CRC32_DIGEST_SIZE);
> > /* If there is no digest in suspend kernel, let it go. */
> > - if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> > + if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> > return false;
> >
> > - ret = get_e820_md5(e820_table_firmware, result);
> > + ret = get_e820_crc32(e820_table_firmware, result);
> > if (ret)
> > return true;
> >
> > - return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> > + return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
> > }
> > #else
> > static int hibernation_e820_save(void *buf)
> > @@ -134,7 +134,7 @@ static int hibernation_e820_save(void *buf)
> >
> > static bool hibernation_e820_mismatch(void *buf)
> > {
> > - /* If md5 is not builtin for restore kernel, let it go. */
> > + /* If crc32 is not builtin for restore kernel, let it go. */
> > return false;
> > }
> > #endif
> > @@ -160,6 +160,9 @@ int arch_hibernation_header_save(void *addr, unsigned
> > int max_size)
> > rdr->jump_address = (unsigned long)restore_registers;
> > rdr->jump_address_phys = __pa_symbol(restore_registers);
> >
> > + /* crc32 digest size is 4 but digest buffer size is 16 so zero it
> > all */
> > + memset(rdr->e820_digest, 0, CRC32_DIGEST_SIZE);
> > +
> > /*
> > * The restore code fixes up CR3 and CR4 in the following sequence:
> > *
> > --
> > 2.18.1
> >
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
wrote:
>
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> This patch changes the integrity check algorithm from md5 to
> crc32. This integrity check is used only to verify accidental
> corruption of the hybernation data
It isn't used for that.
In fact, it is used to detect differences between the memory map used
before hibernation and the one made available by the BIOS during the
subsequent resume. And the check is there, because it is generally
unsafe to load the hibernation image into memory if the current memory
map doesn't match the one used when the image was created.
> and is not intended as a cryptographic integrity check.
But this is true nevertheless, so I would write:
"The purpose of the integrity check is to detect possible differences
between the memory map used at the time when the hibernation image is
about to be loaded into memory and the memory map used at the image
creation time, because it is generally unsafe to load the image if the
current memory map doesn't match the one used when it was created. so
it is not intended as a cryptographic integrity check."
And please make the md5 spelling consistent.
> Md5 is overkill in this case and also disabled in FIPS mode because it
> is known to be broken for cryptographic purposes.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
>
> Tested-by: Dexuan Cui
> Reviewed-by: Dexuan Cui
> Signed-off-by: Chris von Recklinghausen
> ---
> v1 -> v2
>bump up RESTORE_MAGIC
> v2 -> v3
>move embelishment from cover letter to commit comments (no code change)
> v3 -> v4
>add note to comments that md5 isn't used for encryption here.
> v4 -> v5
>reword comment per Simo's suggestion
>
> arch/x86/power/hibernate.c | 35 +++
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..b56172553275 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE 16
>
> struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> - u8 e820_digest[MD5_DIGEST_SIZE];
> + u8 e820_digest[CRC32_DIGEST_SIZE];
> };
>
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
> /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
> *
> * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + * @buf: the crc32 result to be stored to
> */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static int get_e820_crc32(struct e820_table *table, void *buf)
> {
> struct crypto_shash *tfm;
> struct shash_desc *desc;
> int size;
> int ret = 0;
>
> - tfm = crypto_alloc_shash("md5", 0, 0);
> + tfm = crypto_alloc_shash("crc32", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void
> *buf)
>
> static int hibernation_e820_save(void *buf)
> {
> - return get_e820_md5(e820_table_firmware, buf);
> + return get_e820_crc32(e820_table_firmware, buf);
> }
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> int ret;
> - u8 result[MD5_DIGEST_SIZE];
> + u8 result[CRC32_DIGEST_SIZE];
>
> - memset(result, 0, MD5_DIGEST_SIZE);
> + memset(result, 0, CRC32_DIGEST_SIZE);
> /* If there is no digest in suspend kernel, let it go. */
> - if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> + if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> return false;
>
> - ret = get_e820_md5(e820_table_firmware, result);
> + ret = get_e820_crc32(e820_table_firmware, result);
> if (ret)
> return true;
>
> - return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> + return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
> }
> #else
> static int hibernation_e820_save(void *buf)
> @@ -134,15 +134,15 @@ static int hibernation_e820_save(void *buf)
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> - /* If md5 is not builtin for restore kernel, let it go. */
> + /* If crc32 is not builtin for restore kernel, let it go. */
> return false;
> }
> #endif
>
> #ifdef CONFIG_X86_64
> -#define RESTORE_MAGIC 0x23456789ABCDEF01UL
> +#define RESTORE_MAGIC 0x23456789ABCDEF02UL
> #else
> -#define RESTORE_MAGIC 0x12345678UL
> +#define RESTORE_MAGIC 0x12345679UL
> #endif
>
>
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 8, 2021 at 5:26 PM Eric Biggers wrote:
>
> On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> > wrote:
> > >
> > > Suspend fails on a system in fips mode because md5 is used for the e820
> > > integrity check and is not available. Use crc32 instead.
> > >
> > > This patch changes the integrity check algorithm from md5 to
> > > crc32. This integrity check is used only to verify accidental
> > > corruption of the hybernation data
> >
> > It isn't used for that.
> >
> > In fact, it is used to detect differences between the memory map used
> > before hibernation and the one made available by the BIOS during the
> > subsequent resume. And the check is there, because it is generally
> > unsafe to load the hibernation image into memory if the current memory
> > map doesn't match the one used when the image was created.
>
> So what types of "differences" are you trying to detect? If you need to
> detect
> differences caused by someone who maliciously made changes ("malicious"
> implies
> they may try to avoid detection), then you need to use a cryptographic hash
> function (or a cryptographic MAC if the hash value isn't stored separately).
> If
> you only need to detect non-malicious changes (normally these would be called
> "accidental" changes, but sure, it could be changes that are "intentionally"
> made provided that the other side can be trusted to not try to avoid
> detection...)
That's the case here.
> then a non-cryptographic checksum would be sufficient.
Re: [V5 PATCH 1/5] ACPI / scan: Parse _CCA and setup device coherency
On Wednesday, May 20, 2015 05:09:14 PM Suravee Suthikulpanit wrote:
> This patch implements support for ACPI _CCA object, which is introduced in
> ACPIv5.1, can be used for specifying device DMA coherency attribute.
>
> The parsing logic traverses device namespace to parse coherency
> information, and stores it in acpi_device_flags. Then uses it to call
> arch_setup_dma_ops() when creating each device enumerated in DSDT
> during ACPI scan.
>
> This patch also introduces acpi_dma_is_coherent(), which provides
> an interface for device drivers to check the coherency information
> similarly to the of_dma_is_coherent().
>
> Signed-off-by: Mark Salter
> Signed-off-by: Suravee Suthikulpanit
> ---
> drivers/acpi/Kconfig | 3 +++
> drivers/acpi/acpi_platform.c | 2 +-
> drivers/acpi/glue.c | 5 +
> drivers/acpi/scan.c | 35 +++
> include/acpi/acpi_bus.h | 37 -
> include/linux/acpi.h | 5 +
> 6 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ab2cbb5..212735f 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -54,6 +54,9 @@ config ACPI_GENERIC_GSI
> config ACPI_SYSTEM_POWER_STATES_SUPPORT
> bool
>
> +config ACPI_CCA_REQUIRED
> + bool
> +
> config ACPI_SLEEP
> bool
> depends on SUSPEND || HIBERNATION
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 4bf7559..06a67d5 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -103,7 +103,7 @@ struct platform_device
> *acpi_create_platform_device(struct acpi_device *adev)
> pdevinfo.res = resources;
> pdevinfo.num_res = count;
> pdevinfo.fwnode = acpi_fwnode_handle(adev);
> - pdevinfo.dma_mask = DMA_BIT_MASK(32);
> + pdevinfo.dma_mask = acpi_check_dma(adev, NULL) ? DMA_BIT_MASK(32) : 0;
> pdev = platform_device_register_full(&pdevinfo);
> if (IS_ERR(pdev))
> dev_err(&adev->dev, "platform device creation failed: %ld\n",
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 39c485b..b9657af 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
>
> #include "internal.h"
>
> @@ -167,6 +168,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> struct list_head *physnode_list;
> unsigned int node_id;
> int retval = -EINVAL;
> + bool coherent;
>
> if (has_acpi_companion(dev)) {
> if (acpi_dev) {
> @@ -223,6 +225,9 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> if (!has_acpi_companion(dev))
> ACPI_COMPANION_SET(dev, acpi_dev);
>
> + if (acpi_check_dma(acpi_dev, &coherent))
> + arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
> +
Well, so is this going to work for PCI too after all?
> acpi_physnode_link_name(physical_node_name, node_id);
> retval = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
> physical_node_name);
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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: [V5 PATCH 1/5] ACPI / scan: Parse _CCA and setup device coherency
On Friday, May 22, 2015 05:24:15 PM Suravee Suthikulanit wrote:
> Not sure if this went out earlier. So I am resending.
>
> On 5/22/15 16:56, Rafael J. Wysocki wrote:
> >> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> >> >index 39c485b..b9657af 100644
> >> >--- a/drivers/acpi/glue.c
> >> >+++ b/drivers/acpi/glue.c
> >> >@@ -13,6 +13,7 @@
> >> > #include
> >> > #include
> >> > #include
> >> >+#include
> >> >
> >> > #include "internal.h"
> >> >
> >> >@@ -167,6 +168,7 @@ int acpi_bind_one(struct device *dev, struct
> >> >acpi_device *acpi_dev)
> >> > struct list_head *physnode_list;
> >> > unsigned int node_id;
> >> > int retval = -EINVAL;
> >> >+ bool coherent;
> >> >
> >> > if (has_acpi_companion(dev)) {
> >> > if (acpi_dev) {
> >> >@@ -223,6 +225,9 @@ int acpi_bind_one(struct device *dev, struct
> >> >acpi_device *acpi_dev)
> >> > if (!has_acpi_companion(dev))
> >> > ACPI_COMPANION_SET(dev, acpi_dev);
> >> >
> >> >+ if (acpi_check_dma(acpi_dev, &coherent))
> >> >+ arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
> >> >+
> > Well, so is this going to work for PCI too after all?
> >
>
> No, as Bjorn suggested, PCI changes for setting DMA coherent from _CCA
> (patch 3/6 in V4) will be submitted separately. We are working on
> cleaning up and up-streaming the PCI ACPI support for ARM64.
OK, but acpi_bind_one() is called for PCI devices too. Won't that be a problem?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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: [V5 PATCH 1/5] ACPI / scan: Parse _CCA and setup device coherency
On Friday, May 22, 2015 07:15:17 PM Suravee Suthikulanit wrote:
> On 5/22/2015 6:05 PM, Rafael J. Wysocki wrote:
> > On Friday, May 22, 2015 05:24:15 PM Suravee Suthikulanit wrote:
> >> Not sure if this went out earlier. So I am resending.
> >>
> >> On 5/22/15 16:56, Rafael J. Wysocki wrote:
> >>>> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> >>>>> index 39c485b..b9657af 100644
> >>>>> --- a/drivers/acpi/glue.c
> >>>>> +++ b/drivers/acpi/glue.c
> >>>>> @@ -13,6 +13,7 @@
> >>>>> #include
> >>>>> #include
> >>>>> #include
> >>>>> +#include
> >>>>>
> >>>>> #include "internal.h"
> >>>>>
> >>>>> @@ -167,6 +168,7 @@ int acpi_bind_one(struct device *dev, struct
> >>>>> acpi_device *acpi_dev)
> >>>>> struct list_head *physnode_list;
> >>>>> unsigned int node_id;
> >>>>> int retval = -EINVAL;
> >>>>> + bool coherent;
> >>>>>
> >>>>> if (has_acpi_companion(dev)) {
> >>>>> if (acpi_dev) {
> >>>>> @@ -223,6 +225,9 @@ int acpi_bind_one(struct device *dev, struct
> >>>>> acpi_device *acpi_dev)
> >>>>> if (!has_acpi_companion(dev))
> >>>>> ACPI_COMPANION_SET(dev, acpi_dev);
> >>>>>
> >>>>> + if (acpi_check_dma(acpi_dev, &coherent))
> >>>>> + arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
> >>>>> +
> >>> Well, so is this going to work for PCI too after all?
> >>>
> >>
> >> No, as Bjorn suggested, PCI changes for setting DMA coherent from _CCA
> >> (patch 3/6 in V4) will be submitted separately. We are working on
> >> cleaning up and up-streaming the PCI ACPI support for ARM64.
> >
> > OK, but acpi_bind_one() is called for PCI devices too. Won't that be a
> > problem?
> >
> >
> >
> In this case, we would be going through the following call path:
>
>--> pci_device_add()
> |--> pci_dma_configure() ** 1 **
> |--> device_add()
> |--> platform_notify()
> |--> acpi_platform_notify()
> |--> acpi_bind_one() ** 2 **
>
> At (1), we would be calling arch_setup_dma_ops() with the PCI host
> bridge _CCA information. So, it should have already taken care of
> setting up device coherency here.
>
> At (2), if there is no acpi_dev for endpoint devices (which I believe
> this is normally the case), it would return early and skip
> arch_setup_dma_ops().
That's not correct. There may be ACPI companions for endpoint devices too.
> At (2), if there is an acpi_dev, the coherent_dma flag should have
> already been setup by the acpi_init_device_object during ACPI scan.
That one sets the flag for the *ACPI* *companion* of the device, which
I'm still thinking is pointless, isn't it?
> However, I am not certain about this case since I don't have the DSDT
> containing PCI endpoint devices to test with.
Every x86 PC has them (as far as I can say), but in that case there's no
_CCA and they are all coherent.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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: [V6 PATCH 0/7] ACPI: Introduce support for _CCA object
rch/arm64/include/asm/dma-mapping.h | 18 +- > arch/arm64/mm/dma-mapping.c | 92 > +++ > drivers/acpi/Kconfig | 3 + > drivers/acpi/acpi_platform.c | 2 +- > drivers/acpi/glue.c | 5 ++ > drivers/acpi/scan.c | 35 > drivers/base/property.c | 14 + > drivers/crypto/ccp/ccp-platform.c | 60 +--- > drivers/net/ethernet/amd/xgbe/xgbe-main.c | 27 + > drivers/scsi/megaraid/megaraid_sas_fp.c | 8 +++ > drivers/scsi/ufs/unipro.h | 8 +++ > include/acpi/acpi_bus.h | 37 - > include/linux/acpi.h | 5 ++ > include/linux/property.h | 2 + > 15 files changed, 228 insertions(+), 89 deletions(-) I've queued up the series for 4.2, thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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.11-rc1] crypto: Fix boot failure due to module dependency.
On 7/18/2013 11:00 PM, Tim Chen wrote:
On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote:
Tim Chen wrote:
Your approach is quite complicated. I think something simpler like the
following will work:
We cannot benefit from PCLMULQDQ. Is it acceptable for you?
The following code in crct10dif-pclmul_glue.c
static const struct x86_cpu_id crct10dif_cpu_id[] = {
X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
{}
};
MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
will put the module in the device table and get the module
loaded, as long as the cpu support PCLMULQDQ. So we should be able
to benefit.
Excuse me, how can crct10dif-pclmul.ko get loaded automatically?
Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message?
The code:
static const struct x86_cpu_id crct10dif_cpu_id[] = {
X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
{}
};
MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
causes the following line to be added to modules.alias file:
alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul
This should cause udev to load the crct10dif_pclml module when cpu
support the PCLMULQDQ (feature code 0081). I did my testing during
development on 3.10 and the module was indeed loaded.
However, I found that the following commit under 3.11-rc1 broke
the mechanism after some bisection.
commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc
Author: Rafael J. Wysocki
Date: Fri May 3 00:26:22 2013 +0200
ACPI / processor: Use common hotplug infrastructure
Split the ACPI processor driver into two parts, one that is
non-modular, resides in the ACPI core and handles the enumeration
and hotplug of processors and one that implements the rest of the
existing processor driver functionality.
Rafael, can you check and see if this can be fixed so those optimized
crypto modules for Intel cpu that support them can be loaded?
I think this is an ordering issue between udev startup and the time when
devices are registered.
I wonder what happens if you put those modules into the initramfs image?
Rafael
Herbert, we had a discussion earlier on a separate issue regarding the
module load order. We wanted to load all supported crypto t10dif modules
before the crc-t10dif module. You mentioned that the MODULE_ALIAS also
need some fixing and wonder if those changes have been merged into
3.11-rc1? See https://lkml.org/lkml/2013/5/21/216 . Theoretically, that
fix should also get all the crypto modules support t10dif be loaded.
Thanks.
Tim
diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c
b/arch/x86/crypto/crct10dif-pclmul_glue.c
index 7845d7f..a8a95aa 100644
--- a/arch/x86/crypto/crct10dif-pclmul_glue.c
+++ b/arch/x86/crypto/crct10dif-pclmul_glue.c
@@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
static int __init crct10dif_intel_mod_init(void)
{
+ printk(KERN_WARNING "** Checking for X86_FEATURE_PCLMULQDQ\n");
if (!x86_match_cpu(crct10dif_cpu_id))
return -ENODEV;
-
+ printk(KERN_WARNING "** Registering crct10dif-pclmul\n");
return crypto_register_shash(&alg);
}
As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually
adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing
CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y.
So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded,
the pclmulqdq t10dif will have a higher priority and get allocated
and used.
What I'm talking are
(1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher
priority than crct10dif.ko , mkinitramfs will not include
"modprobe crct10dif-pclmul" line in the generated initramfs.
(2) In order to get benefit from PCLMULQDQ, users have to manually make sure
that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is
loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init
script.
(3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded
automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y.
(4) The cause of (3) is that modules.dep does not describe that users will
benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko .
(5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not
supported. This leads to boot failure (since sd_mod.ko cannot be loaded)
if modules.dep says that "crct10dif-pclmul.ko is required by
crc-t10dif.ko".
(6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is
preferred for crc-t10dif.ko but is not required by crc-t10dif.ko".
But there is no such mechanism. Thus, currently available choice is
"allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported"
or "ignore errors by bui
Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency.
On Thursday, July 18, 2013 04:44:20 PM H. Peter Anvin wrote: > On 07/18/2013 03:17 PM, Rafael J. Wysocki wrote: > >> > >> alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > >> > >> This should cause udev to load the crct10dif_pclml module when cpu > >> support the PCLMULQDQ (feature code 0081). I did my testing during > >> development on 3.10 and the module was indeed loaded. > >> > >> However, I found that the following commit under 3.11-rc1 broke > >> the mechanism after some bisection. > >> > >> commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > >> Author: Rafael J. Wysocki > >> Date: Fri May 3 00:26:22 2013 +0200 > >> > >> ACPI / processor: Use common hotplug infrastructure > >> Split the ACPI processor driver into two parts, one that is > >> non-modular, resides in the ACPI core and handles the enumeration > >> and hotplug of processors and one that implements the rest of the > >> existing processor driver functionality. > >> Rafael, can you check and see if this can be fixed so those > >> optimized > >> crypto modules for Intel cpu that support them can be loaded? > > > > I think this is an ordering issue between udev startup and the time when > > devices are registered. > > > > I wonder what happens if you put those modules into the initramfs image? > > > > OK, this bothers me on some pretty deep level... a set of changes > exclusively in drivers/acpi breaking functionality which had nothing to > do with ACPI, specifically CPU-feature-based module loading. Well, they are not exclusively in drivers/acpi, they are in drivers/base/cpu.c too and that most likely is the responsible part. > Please let me know what the investigation comes up with, or if I need to > get more directly involved. I will. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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.11-rc1] crypto: Fix boot failure due to module dependency.
On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote:
> On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote:
> > On 7/18/2013 11:00 PM, Tim Chen wrote:
> > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote:
> > >> Tim Chen wrote:
> > >>>>> Your approach is quite complicated. I think something simpler like
> > >>>>> the
> > >>>>> following will work:
> > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you?
> > >>>
> > >>> The following code in crct10dif-pclmul_glue.c
> > >>>
> > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = {
> > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
> > >>> {}
> > >>> };
> > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
> > >>>
> > >>> will put the module in the device table and get the module
> > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able
> > >>> to benefit.
> > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically?
> > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message?
> > > The code:
> > >
> > > static const struct x86_cpu_id crct10dif_cpu_id[] = {
> > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
> > > {}
> > > };
> > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
> > >
> > > causes the following line to be added to modules.alias file:
> > >
> > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul
> > >
> > > This should cause udev to load the crct10dif_pclml module when cpu
> > > support the PCLMULQDQ (feature code 0081). I did my testing during
> > > development on 3.10 and the module was indeed loaded.
> > >
> > > However, I found that the following commit under 3.11-rc1 broke
> > > the mechanism after some bisection.
> > >
> > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc
> > > Author: Rafael J. Wysocki
> > > Date: Fri May 3 00:26:22 2013 +0200
> > >
> > > ACPI / processor: Use common hotplug infrastructure
> > >
> > > Split the ACPI processor driver into two parts, one that is
> > > non-modular, resides in the ACPI core and handles the enumeration
> > > and hotplug of processors and one that implements the rest of the
> > > existing processor driver functionality.
> > >
> > > Rafael, can you check and see if this can be fixed so those optimized
> > > crypto modules for Intel cpu that support them can be loaded?
> >
> > I think this is an ordering issue between udev startup and the time when
> > devices are registered.
>
> Something that can be fixed?
I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real
breakage in the kernel yet. I need to figure out that part.
> > I wonder what happens if you put those modules into the initramfs image?
>
> Under initramfs image's /lib/modules/3.11.0-rc1/kernel/arch/x86/crypto
> directory? Any files in /lib/modules/3.11.0-rc1/modules.* need to be
> modified?
No, you shouldn't need to modify any files, just please try to make your
mkinitrd (or whatever else you use) put the modules you'd like to be
autoloaded into the image.
That's only something I'd like to know at this point, though, not a
"workaround" or something like that.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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.11-rc1] crypto: Fix boot failure due to module dependency.
On Friday, July 19, 2013 03:03:36 PM Rafael J. Wysocki wrote:
> On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote:
> > On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote:
> > > On 7/18/2013 11:00 PM, Tim Chen wrote:
> > > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote:
> > > >> Tim Chen wrote:
> > > >>>>> Your approach is quite complicated. I think something simpler like
> > > >>>>> the
> > > >>>>> following will work:
> > > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you?
> > > >>>
> > > >>> The following code in crct10dif-pclmul_glue.c
> > > >>>
> > > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = {
> > > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
> > > >>> {}
> > > >>> };
> > > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
> > > >>>
> > > >>> will put the module in the device table and get the module
> > > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able
> > > >>> to benefit.
> > > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically?
> > > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message?
> > > > The code:
> > > >
> > > > static const struct x86_cpu_id crct10dif_cpu_id[] = {
> > > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ),
> > > > {}
> > > > };
> > > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id);
> > > >
> > > > causes the following line to be added to modules.alias file:
> > > >
> > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul
> > > >
> > > > This should cause udev to load the crct10dif_pclml module when cpu
> > > > support the PCLMULQDQ (feature code 0081). I did my testing during
> > > > development on 3.10 and the module was indeed loaded.
> > > >
> > > > However, I found that the following commit under 3.11-rc1 broke
> > > > the mechanism after some bisection.
> > > >
> > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc
> > > > Author: Rafael J. Wysocki
> > > > Date: Fri May 3 00:26:22 2013 +0200
> > > >
> > > > ACPI / processor: Use common hotplug infrastructure
> > > >
> > > > Split the ACPI processor driver into two parts, one that is
> > > > non-modular, resides in the ACPI core and handles the enumeration
> > > > and hotplug of processors and one that implements the rest of the
> > > > existing processor driver functionality.
> > > >
> > > > Rafael, can you check and see if this can be fixed so those optimized
> > > > crypto modules for Intel cpu that support them can be loaded?
> > >
> > > I think this is an ordering issue between udev startup and the time when
> > > devices are registered.
> >
> > Something that can be fixed?
>
> I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real
> breakage in the kernel yet. I need to figure out that part.
OK
I wonder if the appended patch makes the issue go away for you?
Rafael
---
drivers/acpi/acpi_platform.c| 24 +---
drivers/acpi/acpi_processor.c | 14 --
drivers/acpi/processor_driver.c | 26 ++
3 files changed, 31 insertions(+), 33 deletions(-)
Index: linux-pm/drivers/acpi/acpi_processor.c
===
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -397,20 +397,14 @@ static int __cpuinit acpi_processor_add(
result = -ENODEV;
goto err;
}
-
- result = acpi_bind_one(dev, pr->handle);
- if (result)
- goto err;
-
pr->dev = dev;
dev->offline = pr->flags.need_hotplug_init;
- /* Trigger the processor driver's .probe() if present. */
- if (device_attach(dev) >= 0)
- return 1;
+ result = acpi_create_platform_device(device, id);
+ if (result > 0)
+ return result;
- dev_err(dev, "Processor driver could not be attached\n");
- acpi_unbind_one(dev);
+ dev_err(dev, "Unable to create processor platform device\n");
err:
Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency.
On Friday, July 19, 2013 11:08:49 AM Tim Chen wrote: > On Fri, 2013-07-19 at 16:49 +0200, Rafael J. Wysocki wrote: > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > > > the mechanism after some bisection. > > > > > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > > > Author: Rafael J. Wysocki > > > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > > > non-modular, resides in the ACPI core and handles the > > > > > > enumeration > > > > > > and hotplug of processors and one that implements the rest of > > > > > > the > > > > > > existing processor driver functionality. > > > > > > > > > > > > Rafael, can you check and see if this can be fixed so those > > > > > > optimized > > > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > > > > > I think this is an ordering issue between udev startup and the time > > > > > when > > > > > devices are registered. > > > > > > > > Something that can be fixed? > > > > > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > > > breakage in the kernel yet. I need to figure out that part. > > > > OK > > > > I wonder if the appended patch makes the issue go away for you? > > > > Rafael, the patch did fix the cpu feature based module loading issue. > Thanks for your quick response. Alas, this is not the one I'd like to apply. With that patch applied, new device objects are created to avoid binding the processor driver directly to the cpu system device objects, because that apparently confuses udev and it starts to ignore the cpu modalias once the driver has been bound to any of those objects. I've verified in the meantime that this indeed is the case. A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ Greg, I asked you some time ago whether or not it was possible for udev to stop autoloading modules that matched the cpu modalias after a driver had been bound to the cpu system device objects and you said "no". However, this time I can say with certainty that that really is the case. So, the question now is whether or not we can do anything in the kernel to avoid that confusion in udev instead of applying the patch linked above (which is beyond ugly in my not so humble opinion)? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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.11-rc1] crypto: Fix boot failure due to module dependency.
On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > Alas, this is not the one I'd like to apply. > > > > With that patch applied, new device objects are created to avoid binding the > > processor driver directly to the cpu system device objects, because that > > apparently confuses udev and it starts to ignore the cpu modalias once the > > driver has been bound to any of those objects. > > > > I've verified in the meantime that this indeed is the case. > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > Greg, I asked you some time ago whether or not it was possible for udev to > > stop > > autoloading modules that matched the cpu modalias after a driver had been > > bound > > to the cpu system device objects and you said "no". However, this time I > > can > > say with certainty that that really is the case. So, the question now is > > whether or not we can do anything in the kernel to avoid that confusion in > > udev > > instead of applying the patch linked above (which is beyond ugly in my not > > so > > humble opinion)? > > udev isn't doing any module loading, 'modprobe' is just being called for > any new module alias that shows up in the system, and all of the drivers > that match it then get loaded. The problem is that that doesn't happen when a driver is bound to the cpu system device objects. modprobe is just not called for modules that match the cpu modalias in that case. If I call modprobe manually for any of the modules in question, it loads and works no problem. > How is it a problem if a module is attempted to be loaded that is > already loaded? How is it a problem if a different module is loaded for > a device already bound to a driver? Both of those should be total > "no-ops" for the kernel. Precisely, but that's not what happens in practice, hence my question. The situation is this: - With 3.11-rc1 modules that match the CPU modalias are not loaded automatically (that is, modprobe is not called for them by udev) after the processor module is loaded. - With 3.10 they are loaded automatically at any time. - With the patch at https://patchwork.kernel.org/patch/2830561/ on top of 3.11-rc1 the situation is the same as for 3.10. Therefore we have a kernel regression from 3.10 (with respect to the existing user space which is udev) in 3.11-rc1 3.10 and the patch at https://patchwork.kernel.org/patch/2830561/ makes that regression go away. However, that patch is totally voodoo programming, so I wonder if there is any saner alternative or we really need to do voodoo programming in the kernel to work around udev's confusion. > But, I don't know anything about the cpu code, how is loading a module > causing problems? That sounds like it needs to be fixes, as any root > user can load modules whenever they want, you can't protect the kernel > from doing that. Yes, that needs to be fixed, but I don't know *why* it is a problem in the first place. I'd like to understand what's going on, because I don't right now. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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.11-rc1] crypto: Fix boot failure due to module dependency.
On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote:
> On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote:
> > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote:
> > > Alas, this is not the one I'd like to apply.
> > >
> > > With that patch applied, new device objects are created to avoid binding
> > > the
> > > processor driver directly to the cpu system device objects, because that
> > > apparently confuses udev and it starts to ignore the cpu modalias once the
> > > driver has been bound to any of those objects.
> > >
> > > I've verified in the meantime that this indeed is the case.
> > >
> > > A link to the patch in question:
> > > https://patchwork.kernel.org/patch/2830561/
> > >
> > > Greg, I asked you some time ago whether or not it was possible for udev
> > > to stop
> > > autoloading modules that matched the cpu modalias after a driver had been
> > > bound
> > > to the cpu system device objects and you said "no". However, this time I
> > > can
> > > say with certainty that that really is the case. So, the question now is
> > > whether or not we can do anything in the kernel to avoid that confusion
> > > in udev
> > > instead of applying the patch linked above (which is beyond ugly in my
> > > not so
> > > humble opinion)?
> >
> > udev isn't doing any module loading, 'modprobe' is just being called for
> > any new module alias that shows up in the system, and all of the drivers
> > that match it then get loaded.
>
> The problem is that that doesn't happen when a driver is bound to the
> cpu system device objects. modprobe is just not called for modules that
> match the cpu modalias in that case.
>
> If I call modprobe manually for any of the modules in question, it loads
> and works no problem.
OK, I know what's up. udev has no rule that would allow it to load stuff on
the basis of MODALIAS if DRIVER is set in the event properties.
So, the following rule needs to be added to udev rules for things to work
as before:
DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load
$env{MODALIAS}"
To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/
with the following contents:
ACTION=="remove", GOTO="drivers_end"
DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load
$env{MODALIAS}"
LABEL="drivers_end"
but I'm not sure how to propagate this to the udev maintainers.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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.11-rc1] crypto: Fix boot failure due to module dependency.
On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote:
> On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote:
> > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote:
> > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote:
> > > > Alas, this is not the one I'd like to apply.
> > > >
> > > > With that patch applied, new device objects are created to avoid
> > > > binding the
> > > > processor driver directly to the cpu system device objects, because that
> > > > apparently confuses udev and it starts to ignore the cpu modalias once
> > > > the
> > > > driver has been bound to any of those objects.
> > > >
> > > > I've verified in the meantime that this indeed is the case.
> > > >
> > > > A link to the patch in question:
> > > > https://patchwork.kernel.org/patch/2830561/
> > > >
> > > > Greg, I asked you some time ago whether or not it was possible for udev
> > > > to stop
> > > > autoloading modules that matched the cpu modalias after a driver had
> > > > been bound
> > > > to the cpu system device objects and you said "no". However, this time
> > > > I can
> > > > say with certainty that that really is the case. So, the question now
> > > > is
> > > > whether or not we can do anything in the kernel to avoid that confusion
> > > > in udev
> > > > instead of applying the patch linked above (which is beyond ugly in my
> > > > not so
> > > > humble opinion)?
> > >
> > > udev isn't doing any module loading, 'modprobe' is just being called for
> > > any new module alias that shows up in the system, and all of the drivers
> > > that match it then get loaded.
> >
> > The problem is that that doesn't happen when a driver is bound to the
> > cpu system device objects. modprobe is just not called for modules that
> > match the cpu modalias in that case.
> >
> > If I call modprobe manually for any of the modules in question, it loads
> > and works no problem.
>
> OK, I know what's up. udev has no rule that would allow it to load stuff on
> the basis of MODALIAS if DRIVER is set in the event properties.
>
> So, the following rule needs to be added to udev rules for things to work
> as before:
>
> DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load
> $env{MODALIAS}"
>
> To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/
Well, that wasn't a good idea, because the original 80-drivers.rules didn't
work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules
and put this line (alone) into it:
ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load
$env{MODALIAS}"
After that change everything works happily again.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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.11-rc1] crypto: Fix boot failure due to module dependency.
On Saturday, July 20, 2013 11:51:55 AM Rafael J. Wysocki wrote:
> On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote:
> > On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote:
> > > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote:
> > > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote:
> > > > > Alas, this is not the one I'd like to apply.
> > > > >
> > > > > With that patch applied, new device objects are created to avoid
> > > > > binding the
> > > > > processor driver directly to the cpu system device objects, because
> > > > > that
> > > > > apparently confuses udev and it starts to ignore the cpu modalias
> > > > > once the
> > > > > driver has been bound to any of those objects.
> > > > >
> > > > > I've verified in the meantime that this indeed is the case.
> > > > >
> > > > > A link to the patch in question:
> > > > > https://patchwork.kernel.org/patch/2830561/
> > > > >
> > > > > Greg, I asked you some time ago whether or not it was possible for
> > > > > udev to stop
> > > > > autoloading modules that matched the cpu modalias after a driver had
> > > > > been bound
> > > > > to the cpu system device objects and you said "no". However, this
> > > > > time I can
> > > > > say with certainty that that really is the case. So, the question
> > > > > now is
> > > > > whether or not we can do anything in the kernel to avoid that
> > > > > confusion in udev
> > > > > instead of applying the patch linked above (which is beyond ugly in
> > > > > my not so
> > > > > humble opinion)?
> > > >
> > > > udev isn't doing any module loading, 'modprobe' is just being called for
> > > > any new module alias that shows up in the system, and all of the drivers
> > > > that match it then get loaded.
> > >
> > > The problem is that that doesn't happen when a driver is bound to the
> > > cpu system device objects. modprobe is just not called for modules that
> > > match the cpu modalias in that case.
> > >
> > > If I call modprobe manually for any of the modules in question, it loads
> > > and works no problem.
> >
> > OK, I know what's up. udev has no rule that would allow it to load stuff on
> > the basis of MODALIAS if DRIVER is set in the event properties.
> >
> > So, the following rule needs to be added to udev rules for things to work
> > as before:
> >
> > DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load
> > $env{MODALIAS}"
> >
> > To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/
>
> Well, that wasn't a good idea, because the original 80-drivers.rules didn't
> work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules
> and put this line (alone) into it:
>
> ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod
> load $env{MODALIAS}"
I made a mistake in the above, which should be:
ACTION=="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod
load $env{MODALIAS}"
sorry about that.
> After that change everything works happily again.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
On Sunday, September 15, 2013 08:56:46 AM Lee, Chun-Yi wrote: > Hi experts, > > This patchset is the implementation for signature verification of hibernate > snapshot image. The origin idea is from Jiri Kosina: Let EFI bootloader > generate key-pair in UEFI secure boot environment, then pass it to kernel > for sign/verify S4 image. > > Due to there have potential threat from the S4 image hacked, it may causes > kernel lost the trust in UEFI secure boot. Hacker attack the S4 snapshot > image in swap partition through whatever exploit from another trusted OS, > and the exploit may don't need physical access machine. > > So, this patchset give the ability to kernel for parsing RSA private key > from EFI bootloader, then using the private key to generate the signature > of S4 snapshot image. Kernel put the signature to snapshot header, and > verify the signature when kernel try to recover snapshot image to memory. I wonder what the status of this work is? Is it considered ready for inclusion or are you still going to work on it and resubmit? Rafael -- 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 00/35] treewide trivial patches converting pr_warning to pr_warn
On Fri, Feb 17, 2017 at 8:11 AM, Joe Perches wrote: > There are ~4300 uses of pr_warn and ~250 uses of the older > pr_warning in the kernel source tree. > > Make the use of pr_warn consistent across all kernel files. > > This excludes all files in tools/ as there is a separate > define pr_warning for that directory tree and pr_warn is > not used in tools/. > > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing. Sorry about asking if that has been asked already. Wouldn't it be slightly less intrusive to simply redefined pr_warning() as a synonym for pr_warn()? Thanks, Rafael
Re: PBKDF2 support in the linux kernel
On Thursday, May 24, 2018 7:57:37 AM CEST Stephan Mueller wrote: > Am Donnerstag, 24. Mai 2018, 04:45:00 CEST schrieb Eric Biggers: > > Hi Eric, > > > > > "Not having to rely on any third-party library" is not an excuse to add > > random code to the kernel, which runs in a privileged context. Please do > > PBKDF2 in userspace instead. > > I second that. Besides, if you really need to rely on the kernel crypto API > to > do that because you do not want to add yet another crypto lib, libkcapi has a > PBKDF2 implementation that uses the kernel crypto API via AF_ALG. I.e. the > kernel crypto API is used and yet the PBKDF logic is in user space. > > http://www.chronox.de/libkcapi.html So the problem is that Yu would like to use this for hibernation encryption done entirely in the kernel. The exact use case is to generate a symmetric encryption key out of a passphrase. Is there a recommended way to do that using the algorithms already implemented in the kernel? Thanks, Rafael
Re: PBKDF2 support in the linux kernel
On Thursday, May 24, 2018 11:11:32 AM CEST Stephan Mueller wrote: > Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki: > > Hi Rafael, Hi Stephan, > > So the problem is that Yu would like to use this for hibernation encryption > > done entirely in the kernel. > > But why do you need to perform PBKDF in kernel space? > > If you retain the password information in the kernel, you could retain the > derived key instead of the passcode. > > If, however, you ask for the user password during resume, you need some user > space component to query that password. The PBKDF can also be handled in user > space along with the query. In principle it is possible to pass a key to the kernel from user space, but that doesn't guarantee the key to be a really good one. It depends on a user space component to do the right thing and IMO that is risky. And please note that the information contained in hibernation images may be extremely sensitive (keys and similar). > Or how do you want to handle the passcode? The idea is to write a passphrase to the kernel via something like sysfs, generate a key from it on the fly and store the key. > > > > The exact use case is to generate a symmetric encryption key out of a > > passphrase. Is there a recommended way to do that using the algorithms > > already implemented in the kernel? > > For example, dmcrypt uses PBKDF2 for its operation. And this PBKDF is done in > user space by libcryptsetup that utilizes a crypto lib, commonly libgcrypt. I know that. We can do that here too in principle, but I'd prefer all crypto to take place in the kernel in this particular case. Thanks, Rafael
Re: PBKDF2 support in the linux kernel
On Thursday, May 24, 2018 11:36:04 AM CEST Stephan Mueller wrote: > Am Donnerstag, 24. Mai 2018, 11:27:56 CEST schrieb Rafael J. Wysocki: > > Hi Rafael, > > > On Thursday, May 24, 2018 11:11:32 AM CEST Stephan Mueller wrote: > > > Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki: > > > > > > Hi Rafael, > > > > Hi Stephan, > > > > > > So the problem is that Yu would like to use this for hibernation > > > > encryption > > > > done entirely in the kernel. > > > > > > But why do you need to perform PBKDF in kernel space? > > > > > > If you retain the password information in the kernel, you could retain the > > > derived key instead of the passcode. > > > > > > If, however, you ask for the user password during resume, you need some > > > user space component to query that password. The PBKDF can also be > > > handled in user space along with the query. > > > > In principle it is possible to pass a key to the kernel from user > > space, but that doesn't guarantee the key to be a really good one. It > > depends on a user space component to do the right thing and IMO that is > > risky. > > > > And please note that the information contained in hibernation images may > > be extremely sensitive (keys and similar). > > > > > Or how do you want to handle the passcode? > > > > The idea is to write a passphrase to the kernel via something like sysfs, > > generate a key from it on the fly and store the key. > > But what is the difference between a passcode and the key derived from it. > The > derived key should only spread the assumed entropy in the passcode evenly. In > addition with its iteration count, it shall ensure that offline brute force > attacks of the passcode using stored protected data is made hard. > > So, I do not see why it is risky to do the PBKDF in user space and then hand > the key to the kernel at runtime. It does not really matter if the attacker > sees the plaintext key or the passcode. If the attacker can access the kernel/ > user communication channel, we have a problem in any case. OK, we'll follow this advice, thanks! Best, Rafael
Re: PBKDF2 support in the linux kernel
On Saturday, May 26, 2018 7:12:36 AM CEST Herbert Xu wrote: > On Tue, May 22, 2018 at 11:00:40AM +0800, Yu Chen wrote: > > Hi all, > > The request is that, we'd like to generate a symmetric key derived from > > user provided passphase(not rely on any third-party library). May I know if > > there is a PBKDF2(Password-Based Key Derivation Function 2) support in the > > kernel? (https://tools.ietf.org/html/rfc2898#5.2) > > We have hmac sha1 in the kernel, do we have plan to port/implement > > corresponding PBKDF2 in the kernel too? > > The rule for adding crypto code to the kernel is simple, there > must be an in-kernel user of the algorithm. So we are talking about an in-kernel user here. Again, the PBKDF2 user would be hibernation. It could either take a key from user space, which would require a key-generating user-space component to be present in the initramfs (I guess no issue for a regular distro, but I can imagine cases when it may be a difficulty), or take a passphrase from user space and generate a key by itself (that's what we would like to use PBKDF2 for). We would prefer the latter for various reasons (convenience mostly, but also not having to rely on user space to do the right thing). Thanks, Rafael
Re: PBKDF2 support in the linux kernel
On Sunday, May 27, 2018 2:33:33 PM CEST Theodore Y. Ts'o wrote: > On Sun, May 27, 2018 at 01:22:05PM +0200, Rafael J. Wysocki wrote: > > Again, the PBKDF2 user would be hibernation. It could either take a key > > from > > user space, which would require a key-generating user-space component to be > > present in the initramfs (I guess no issue for a regular distro, but I can > > imagine cases when it may be a difficulty), or take a passphrase from user > > space and generate a key by itself (that's what we would like to use PBKDF2 > > for). > > Right, but are you going to get the passphrase from user space? You > have to prompt from user space anyway, Right. > so running PBPDF2 from > userspace isn't that big of deal. Feel free to grab the > implementation from e2fsprogs; it's not hard. :-) So that's what we'll probably end up doing.
Re: [PATCH v6 10/18] x86/power/64: Remove VLA usage
On Tue, Jul 24, 2018 at 6:49 PM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> removes the discouraged use of AHASH_REQUEST_ON_STACK by switching to
> shash directly and allocating the descriptor in heap memory (which should
> be fine: the tfm has already been allocated there too).
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
> Acked-by: Pavel Machek
I think I can queue this up if there are no objections from others.
Do you want me to do that?
> ---
> arch/x86/power/hibernate_64.c | 36 ---
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
> index 67ccf64c8bd8..f8e3b668d20b 100644
> --- a/arch/x86/power/hibernate_64.c
> +++ b/arch/x86/power/hibernate_64.c
> @@ -233,29 +233,35 @@ struct restore_data_record {
> */
> static int get_e820_md5(struct e820_table *table, void *buf)
> {
> - struct scatterlist sg;
> - struct crypto_ahash *tfm;
> + struct crypto_shash *tfm;
> + struct shash_desc *desc;
> int size;
> int ret = 0;
>
> - tfm = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
> + tfm = crypto_alloc_shash("md5", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> - {
> - AHASH_REQUEST_ON_STACK(req, tfm);
> - size = offsetof(struct e820_table, entries) + sizeof(struct
> e820_entry) * table->nr_entries;
> - ahash_request_set_tfm(req, tfm);
> - sg_init_one(&sg, (u8 *)table, size);
> - ahash_request_set_callback(req, 0, NULL, NULL);
> - ahash_request_set_crypt(req, &sg, buf, size);
> -
> - if (crypto_ahash_digest(req))
> - ret = -EINVAL;
> - ahash_request_zero(req);
> + desc = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm),
> + GFP_KERNEL);
> + if (!desc) {
> + ret = -ENOMEM;
> + goto free_tfm;
> }
> - crypto_free_ahash(tfm);
>
> + desc->tfm = tfm;
> + desc->flags = 0;
> +
> + size = offsetof(struct e820_table, entries) +
> + sizeof(struct e820_entry) * table->nr_entries;
> +
> + if (crypto_shash_digest(desc, (u8 *)table, size, buf))
> + ret = -EINVAL;
> +
> + kzfree(desc);
> +
> +free_tfm:
> + crypto_free_shash(tfm);
> return ret;
> }
>
> --
> 2.17.1
>
Re: [PATCH v6 10/18] x86/power/64: Remove VLA usage
On Wednesday, July 25, 2018 8:01:47 PM CEST Kees Cook wrote: > On Wed, Jul 25, 2018 at 4:32 AM, Rafael J. Wysocki wrote: > > On Tue, Jul 24, 2018 at 6:49 PM, Kees Cook wrote: > >> In the quest to remove all stack VLA usage from the kernel[1], this > >> removes the discouraged use of AHASH_REQUEST_ON_STACK by switching to > >> shash directly and allocating the descriptor in heap memory (which should > >> be fine: the tfm has already been allocated there too). > >> > >> [1] > >> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com > >> > >> Signed-off-by: Kees Cook > >> Acked-by: Pavel Machek > > > > I think I can queue this up if there are no objections from others. > > > > Do you want me to do that? > > Sure thing. It looks like the other stand-alone patches like this one > are getting taken into the non-crypto trees, so that's fine. > Applied now, thanks!
