Re: [PATCH 1/1 v9] use crc32 instead of md5 for hibernation e820 integrity check
On Fri, Apr 16, 2021 at 09:16:55AM -0400, Chris von Recklinghausen wrote:
> Hibernation fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
Doesn't the commit title need to be prefixed with something like "x86/power"?
> The check is intended to detect whether the E820 memory map provided
> by the firmware after cold boot unexpectedly differs from the one that
> was in use when the hibernation image was created. In this case, the
> hibernation image cannot be restored, as it may cover memory regions
> that are no longer available to the OS.
>
> A non-cryptographic checksum such as CRC-32 is sufficient to detect such
> inadvertent deviations.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
The "Fixes" line shouldn't be line-wrapped.
Otherwise this looks fine. The explanation in the commit message still isn't
great, but it's much better than it was before.
You can add:
Reviewed-by: Eric Biggers
- Eric
Re: [PATCH v8 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 15, 2021 at 03:46:46PM -0400, Chris von Recklinghausen wrote: > Hibernation 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. The second paragraph is redundant with the first. > /** > - * get_e820_md5 - calculate md5 according to given e820 table > + * compute_e820_crc32 - calculate md5 according to given e820 table > * > * @table: the e820 table to be calculated > - * @buf: the md5 result to be stored to > */ This comment still mentions MD5. Also, this isn't a well-formed kerneldoc comment, since it doesn't document the return value. Also, this function is calculating the checksum *of* the table, not calculating a checksum "according to" it (whatever that means). Something like this would be good, I think: /** * compute_e820_crc32 - compute the CRC-32 of the given e820 table * * @table: the e820 table to be checksummed * * Return: the resulting checksum */ Also, please try 'git grep -i md5 arch/x86/kernel/'. There is still another reference to MD5 that should be updated, in arch/x86/kernel/e820.c. - Eric
Re: [PATCH v2 8/8] block: add WARN() in bio_split() for sector alignment
On Thu, Mar 25, 2021 at 09:26:09PM +, Satya Tangirala wrote: > The number of sectors passed to bio_split() should be aligned to > bio_required_sector_alignment(). All callers (other than bounce.c) have > been updated to ensure this, so add a WARN() if the number of sectors is > not aligned. (bounce.c was not updated since it's legacy code that > won't interact with inline encryption). What does bounce.c "won't interact with inline encryption" mean, exactly? Devices that enable bounce buffering won't declare inline encryption support, so bounce.c will never see a bio that has an encryption context? > diff --git a/block/bio.c b/block/bio.c > index 26b7f721cda8..cb348f134a15 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1458,6 +1458,7 @@ struct bio *bio_split(struct bio *bio, int sectors, > > BUG_ON(sectors <= 0); > BUG_ON(sectors >= bio_sectors(bio)); > + WARN_ON(!IS_ALIGNED(sectors, bio_required_sector_alignment(bio))); If this warning triggers, shouldn't the function return NULL to indicate a failure rather than proceeding on? - Eric
Re: [PATCH v2 6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
On Thu, Mar 25, 2021 at 09:26:07PM +, Satya Tangirala wrote:
> Not all crypto data unit sizes might be supported by the block layer due to
> certain queue limits. This new function checks the queue limits and
> appropriately modifies the keyslot manager to reflect only the supported
> crypto data unit sizes. blk_ksm_register() runs any given ksm through this
> function before actually registering the ksm with a queue.
>
> Signed-off-by: Satya Tangirala
> ---
> block/keyslot-manager.c | 59 +
> 1 file changed, 59 insertions(+)
>
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 2a2b1a9785d2..fad6d9c4b649 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -450,12 +450,71 @@ bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm)
> }
> EXPORT_SYMBOL_GPL(blk_ksm_is_empty);
>
> +/*
> + * Restrict the supported data unit sizes of the ksm based on the request
> queue
> + * limits
> + */
> +void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
> + struct queue_limits *limits)
As the kernel test robot hinted at, this function needs to be 'static'.
> +{
> + /* The largest possible data unit size we support is PAGE_SIZE. */
> + unsigned long largest_dus = PAGE_SIZE;
> + unsigned int dus_allowed_mask;
> + int i;
> + bool dus_was_restricted = false;
> +
> + /*
> + * If the queue doesn't support SG gaps, a bio might get split in the
> + * middle of a data unit. So require SG gap support for inline
> + * encryption for any data unit size larger than a single sector.
> + */
> + if (limits->virt_boundary_mask)
> + largest_dus = SECTOR_SIZE;
> +
> + /*
> + * If the queue has chunk_sectors, the bio might be split within a data
> + * unit if the data unit size is larger than a single sector. So only
> + * support a single sector data unit size in this case.
> + */
> + if (limits->chunk_sectors)
> + largest_dus = SECTOR_SIZE;
So in practice, this means that inline encryption will be disabled on any disk
that declares a virt_boundary_mask or chunk_sectors.
What are the real-world consequences of that? Will that have any consequences
for UFS or eMMC, or are those things never applicable to UFS or eMMC?
It would also be helpful if the comments explained why these restrictions are
necessary. They kind of do, but they don't explicitly give an example --
presumably the issue is that a crypto data unit could cross a virt_boundary_mask
or chunk_sectors boundary?
> + /*
> + * Any bio sent to the queue must be allowed to contain at least a
> + * data_unit_size worth of data. Since each segment in a bio contains
> + * at least a SECTOR_SIZE worth of data, it's sufficient that
> + * queue_max_segments(q) * SECTOR_SIZE >= data_unit_size. So disable
> + * all data_unit_sizes not satisfiable.
> + */
> + largest_dus = min(largest_dus,
> + 1UL << (fls(limits->max_segments) - 1 + SECTOR_SHIFT));
> +
> + /* Clear all unsupported data unit sizes. */
> + dus_allowed_mask = (largest_dus << 1) - 1;
> + for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
> + if (ksm->crypto_modes_supported[i] & (~dus_allowed_mask))
> + dus_was_restricted = true;
> + ksm->crypto_modes_supported[i] &= dus_allowed_mask;
> + }
So again in practice, this effectively disables inline encryption on any disk
that doesn't declare max_segments >= 8. What are the real-world consequences of
that -- will this ever be a problem for UFS or eMMC?
Also, why is it necessary to assume the worst case of 512 bytes per segment?
> + if (dus_was_restricted) {
> + pr_warn("Disallowed use of encryption data unit sizes above %lu
> bytes with inline encryption hardware because of device request queue
> limits.\n",
> + largest_dus);
> + }
Could this message include the disk that it is talking about?
> bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue
> *q)
> {
> if (blk_integrity_queue_supports_integrity(q)) {
> pr_warn("Integrity and hardware inline encryption are not
> supported together. Disabling hardware inline encryption.\n");
> return false;
> }
> +
> + blk_ksm_restrict_dus_to_queue_limits(ksm, >limits);
> +
> + if (blk_ksm_is_empty(ksm))
> + return false;
> +
> q->ksm = ksm;
> return true;
> }
Adding a kerneldoc comment to this function would be helpful. Especially to
explain what a return value of false means, exactly.
- Eric
Re: [PATCH v2 5/8] block: respect bio_required_sector_alignment() in blk-crypto-fallback
On Thu, Mar 25, 2021 at 09:26:06PM +, Satya Tangirala wrote: > Make blk_crypto_split_bio_if_needed() respect > bio_required_sector_alignment() when calling bio_split(). > A bit more explanation would be helpful here. Does this fix something, and if so what is it and under what circumstances? - Eric
Re: [PATCH v2 4/8] block: introduce bio_required_sector_alignment()
On Thu, Mar 25, 2021 at 09:26:05PM +, Satya Tangirala wrote:
> This function returns the required alignment for the number of sectors in
> a bio. In particular, the number of sectors passed to bio_split() must be
> aligned to this value.
>
> Signed-off-by: Satya Tangirala
> ---
> block/blk.h | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/block/blk.h b/block/blk.h
> index 3b53e44b967e..5ef207a6d34c 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -261,6 +261,18 @@ static inline unsigned int
> bio_allowed_max_sectors(struct request_queue *q)
> return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9;
> }
>
> +/*
> + * The required sector alignment for a bio. The number of sectors in any bio
> + * that's constructed/split must be aligned to this value.
What does "constructed" mean in this context?
> + */
> +static inline unsigned int bio_required_sector_alignment(struct bio *bio)
> +{
> + struct request_queue *q = bio->bi_bdev->bd_disk->queue;
> +
> + return max(queue_logical_block_size(q) >> SECTOR_SHIFT,
> +blk_crypto_bio_sectors_alignment(bio));
> +}
It would be slightly simpler to use bdev_logical_block_size(bio->bi_bdev)
instead of queue_logical_block_size(bio->bi_bdev->bd_disk->queue).
Also, if there's interest in avoiding the branch implied by the max() here, it
would be possible to take advantage of the values being powers of 2 as follows:
static inline unsigned int bio_required_sector_alignment(struct bio *bio)
{
unsigned int alignmask =
(bdev_logical_block_size(q) >> SECTOR_SHIFT) - 1;
alignmask |= blk_crypto_bio_sectors_alignment(bio) - 1;
return alignmask + 1;
}
Re: [PATCH v2 4/8] block: introduce bio_required_sector_alignment()
On Tue, Mar 30, 2021 at 07:06:53PM +0100, Christoph Hellwig wrote:
> On Thu, Mar 25, 2021 at 09:26:05PM +, Satya Tangirala wrote:
> > +/*
> > + * The required sector alignment for a bio. The number of sectors in any
> > bio
> > + * that's constructed/split must be aligned to this value.
> > + */
> > +static inline unsigned int bio_required_sector_alignment(struct bio *bio)
> > +{
> > + struct request_queue *q = bio->bi_bdev->bd_disk->queue;
> > +
> > + return max(queue_logical_block_size(q) >> SECTOR_SHIFT,
> > + blk_crypto_bio_sectors_alignment(bio));
> > +}
>
> It might make more sense to just have a field in the request queue
> for the max alignment so that the fast path just looks at one field.
> Then the various setup time functions would update it to the maximum
> required.
I don't see how that would work here, as the required alignment is a per-bio
thing which depends on whether the bio has an encryption context or not, and (if
it does) also the data_unit_size the submitter of the bio selected.
We could just always assume the worst-case scenario, but that seems
unnecessarily pessimistic?
- Eric
Re: [PATCH v2 3/8] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment()
On Thu, Mar 25, 2021 at 09:26:04PM +, Satya Tangirala wrote: > The size of any bio must be aligned to the data unit size of the bio crypt > context (if it exists) of that bio. This must also be ensured whenever a > bio is split. Introduce blk_crypto_bio_sectors_alignment() that returns > the required alignment in sectors. The number of sectors passed to > any call of bio_split() should be aligned to > blk_crypto_bio_sectors_alignment(). "should be aligned" => "must be aligned"? > +/* > + * Returns the alignment requirement for the number of sectors in this bio > based > + * on its bi_crypt_context. Any bios split from this bio must follow this > + * alignment requirement as well. > + */ It would be helpful to expand this comment a bit to explictly mention that the size of the bio must be a multiple of the crypto data unit size that was selected by the submitter of the bio, which is the granularity of encryption/decryption. Keep in mind that people reading this code won't necessarily be familiar with inline encryption. - Eric
Re: [PATCH v2 2/8] dm,mmc,ufshcd: handle error from blk_ksm_register()
On Thu, Mar 25, 2021 at 09:26:03PM +, Satya Tangirala wrote:
> Handle any error from blk_ksm_register() in the callers. Previously,
> the callers ignored the return value because blk_ksm_register() wouldn't
> fail as long as the request_queue didn't have integrity support too, but
> as this is no longer the case, it's safer for the callers to just handle
> the return value appropriately.
>
> Signed-off-by: Satya Tangirala
> ---
> drivers/md/dm-table.c| 3 ++-
> drivers/mmc/core/crypto.c| 6 --
> drivers/scsi/ufs/ufshcd-crypto.c | 6 --
> 3 files changed, 10 insertions(+), 5 deletions(-)
This probably should be 3 patches, one for each subsystem.
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index db18a58adad7..1225b9050f29 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1372,7 +1372,8 @@ static void dm_update_keyslot_manager(struct
> request_queue *q,
>
> /* Make the ksm less restrictive */
> if (!q->ksm) {
> - blk_ksm_register(t->ksm, q);
> + if (WARN_ON(!blk_ksm_register(t->ksm, q)))
> + dm_destroy_keyslot_manager(t->ksm);
> } else {
> blk_ksm_update_capabilities(q->ksm, t->ksm);
> dm_destroy_keyslot_manager(t->ksm);
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index 419a368f8402..616103393557 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -21,8 +21,10 @@ void mmc_crypto_set_initial_state(struct mmc_host *host)
>
> void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
> {
> - if (host->caps2 & MMC_CAP2_CRYPTO)
> - blk_ksm_register(>ksm, q);
> + if (host->caps2 & MMC_CAP2_CRYPTO) {
> + if (WARN_ON(!blk_ksm_register(>ksm, q)))
> + host->caps2 &= ~MMC_CAP2_CRYPTO;
> + }
> }
> EXPORT_SYMBOL_GPL(mmc_crypto_setup_queue);
>
> diff --git a/drivers/scsi/ufs/ufshcd-crypto.c
> b/drivers/scsi/ufs/ufshcd-crypto.c
> index d70cdcd35e43..f47a72fefe9e 100644
> --- a/drivers/scsi/ufs/ufshcd-crypto.c
> +++ b/drivers/scsi/ufs/ufshcd-crypto.c
> @@ -233,6 +233,8 @@ void ufshcd_init_crypto(struct ufs_hba *hba)
> void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba,
> struct request_queue *q)
> {
> - if (hba->caps & UFSHCD_CAP_CRYPTO)
> - blk_ksm_register(>ksm, q);
> + if (hba->caps & UFSHCD_CAP_CRYPTO) {
> + if (WARN_ON(!blk_ksm_register(>ksm, q)))
> + hba->caps &= ~UFSHCD_CAP_CRYPTO;
> + }
It would be helpful to add a comment in each case to explain why the WARN_ON
should never trigger.
Also, clearing UFSHCD_CAP_CRYPTO or MMC_CAP2_CRYPTO doesn't really make sense
here because those capabilities apply to the whole UFS or MMC host controller,
not just to the individual request_queue which failed. (A host controller can
control multiple devices, each of which has its own request_queue.) Isn't
blk_ksm_register() failing already enough to ensure that the inline crypto
support isn't used on that particular request_queue? What is the benefit of
clearing UFSHCD_CAP_CRYPTO and MMC_CAP2_CRYPTO too?
- Eric
Re: [PATCH v2 1/8] block: introduce blk_ksm_is_empty()
On Thu, Mar 25, 2021 at 09:26:02PM +, Satya Tangirala wrote:
> This function checks if a given keyslot manager supports any encryption
> mode/data unit size combination (and returns true if there is no such
> supported combination). Helps clean up code a little.
>
> Signed-off-by: Satya Tangirala
> ---
> block/keyslot-manager.c | 13 +
> drivers/md/dm-table.c | 11 +--
> include/linux/keyslot-manager.h | 2 ++
> 3 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 2c4a55bea6ca..2a2b1a9785d2 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -437,6 +437,19 @@ void blk_ksm_destroy(struct blk_keyslot_manager *ksm)
> }
> EXPORT_SYMBOL_GPL(blk_ksm_destroy);
>
> +bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm)
> +{
I agree with Christoph that this could use a kerneldoc comment.
> diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
> index a27605e2f826..5bf0cea20c81 100644
> --- a/include/linux/keyslot-manager.h
> +++ b/include/linux/keyslot-manager.h
> @@ -117,4 +117,6 @@ bool blk_ksm_is_superset(struct blk_keyslot_manager
> *ksm_superset,
> void blk_ksm_update_capabilities(struct blk_keyslot_manager *target_ksm,
>struct blk_keyslot_manager *reference_ksm);
>
> +bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm);
> +
It's easier to read if declarations are kept in the same order as the
definitions.
- Eric
Re: [GIT PULL][PATCH v9 0/3] Update to zstd-1.4.10
On Wed, Apr 14, 2021 at 11:53:51AM -0700, Nick Terrell wrote: > On Wed, Apr 14, 2021 at 11:35 AM Eric Biggers wrote: > > > > On Wed, Apr 14, 2021 at 11:01:29AM -0700, Nick Terrell wrote: > > > Hi all, > > > > > > I would really like to make some progress on this and get it merged. > > > This patchset offsers: > > > * 15-30% better decompression speed > > > * 3 years of zstd bug fixes and code improvements > > > * Allows us to import zstd directly from upstream so we don't fall 3 > > > years out of date again > > > > > > Thanks, > > > Nick > > > > > > > I think it would help get it merged if someone actually volunteered to > > maintain > > it. As-is there is no entry in MAINTAINERS for this code. > > I was discussing with Chris Mason about volunteering to maintain the > code myself. > We wanted to wait until this series got merged before going that > route, because there > was already a lot of comments about it, and I didn't want to appear to > be trying to bypass > any review or criticisms. But, please let me know what you think. > I expect that most people would like to see a commitment to maintain this code before merging. The usual way to do that is to add a MAINTAINERS entry. Otherwise it is 27000 lines of code dumped on other people to maintain. - Eric
Re: [GIT PULL][PATCH v9 0/3] Update to zstd-1.4.10
On Wed, Apr 14, 2021 at 11:01:29AM -0700, Nick Terrell wrote: > Hi all, > > I would really like to make some progress on this and get it merged. > This patchset offsers: > * 15-30% better decompression speed > * 3 years of zstd bug fixes and code improvements > * Allows us to import zstd directly from upstream so we don't fall 3 > years out of date again > > Thanks, > Nick > I think it would help get it merged if someone actually volunteered to maintain it. As-is there is no entry in MAINTAINERS for this code. - Eric
Re: [PATCH v7 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Tue, Apr 13, 2021 at 12:13:30PM -0400, Chris von Recklinghausen wrote:
> Suspend fails on a system in fips mode because md5 is used for the e820
Suspend to disk (hibernation), or any suspend?
> 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];
> + unsigned long e820_digest;
> };
This field should probably be renamed to 'e820_crc' or 'e820_checksum', since
"digest" normally means a cryptographic digest.
- Eric
Re: [PATCH v7 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Tue, Apr 13, 2021 at 12:13:30PM -0400, Chris von Recklinghausen wrote:
> +static inline void get_e820_crc32(struct e820_table *table, void *buf)
> {
This should just return the CRC-32 value as a u32. There's no need for the
'void *buf' argument.
Also like I said, compute_e820_crc32() would be a more logical name.
> @@ -179,7 +133,8 @@ int arch_hibernation_header_save(void *addr, unsigned int
> max_size)
>*/
> rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK;
>
> - return hibernation_e820_save(rdr->e820_digest);
> + hibernation_e820_save(>e820_digest);
> + return 0;
Like I said, hibernation_e820_save() should just be inlined into here:
rdr->e820_digest = compute_e820_crc32(e820_table_firmware)
Having the helper function doesn't add anything.
> /**
> @@ -200,7 +155,7 @@ int arch_hibernation_header_restore(void *addr)
> jump_address_phys = rdr->jump_address_phys;
> restore_cr3 = rdr->cr3;
>
> - if (hibernation_e820_mismatch(rdr->e820_digest)) {
> + if (hibernation_e820_mismatch(>e820_digest)) {
Likewise, this should be just
if (rdr->e820_digest != compute_e820_crc32(e820_table_firmware)) {
- Eric
Re: [PATCH v6 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote: > On 4/12/21 1:45 PM, Eric Biggers wrote: > > On Mon, Apr 12, 2021 at 10:09:32AM -0400, 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. > > > > > > 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. > > This still doesn't actually explain why a non-cryptographic checksum is > > sufficient. "Detection of possible differences" could very well require > > cryptographic authentication; it depends on whether malicious changes need > > to be > > detected or not. > > Hi Eric, > > The cases that the commit comments for 62a03defeabd mention are the same as > for this patch, e.g. > > 1. Without this patch applied, it is possible that BIOS has > provided an inconsistent memory map, but the resume kernel is still > able to restore the image anyway(e.g, E820_RAM region is the superset > of the previous one), although the system might be unstable. So this > patch tries to treat any inconsistent e820 as illegal. > > 2. Another case is, this patch replies on comparing the e820_saved, but > currently the e820_save might not be strictly the same across > hibernation, even if BIOS has provided consistent e820 map - In > theory mptable might modify the BIOS-provided e820_saved dynamically > in early_reserve_e820_mpc_new, which would allocate a buffer from > E820_RAM, and marks it from E820_RAM to E820_RESERVED). > This is a potential and rare case we need to deal with in OS in > the future. > > Maybe they should be added to the comments with this patch as well? In any > case, the above comments only mention detecting consequences of BIOS > issues/actions on the e820 map and not intrusions from attackers requiring > cryptographic protection. Does that seem to be a reasonable explanation to > you? If so I can add these to the commit comments. > > I'll make the other changes you suggest below. > > Thanks, > Those details are still missing the high-level point. Is this just meant to detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to detect malicious changes? That's all that really needs to be mentioned. - Eric
Re: [PATCH v6 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Mon, Apr 12, 2021 at 10:09:32AM -0400, 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.
>
> 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.
This still doesn't actually explain why a non-cryptographic checksum is
sufficient. "Detection of possible differences" could very well require
cryptographic authentication; it depends on whether malicious changes need to be
detected or not.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
>
> 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
> v5 -> v6
>use wording from Eric Biggers, use crc32_le instead of crc32 from crypto
> framework (crc32_le is in the core API and removes need for #defines)
>
> arch/x86/power/hibernate.c | 76 +++---
> 1 file changed, 22 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..f39e507e34ca 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -13,6 +13,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
crypto/hash.h is no longer needed.
>
> @@ -55,94 +57,60 @@ int pfn_is_nosave(unsigned long pfn)
> }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE (sizeof (u32))
>
> 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];
> };
It would be simpler to just make this field 'unsigned long'. Then there would
be no need to deal with memcpy().
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
Calling this "compute_e820_crc32()" would avoid confusion with retrieving the
previously-stored crc32 value.
> + ret = crc32_le(0, (unsigned char const *)table, size);
It would be better to do ~crc32_le(~0, ...) (i.e., invert at beginning and end)
to match the recommended usage of CRC-32. Unfortunately the library function
doesn't do the inversions by default.
> static int hibernation_e820_save(void *buf)
> {
> - return get_e820_md5(e820_table_firmware, buf);
> + return get_e820_crc32(e820_table_firmware, buf);
> }
This should be inlined into arch_hibernation_header_save(). Also note that it
can no longer fail.
>
> static bool hibernation_e820_mismatch(void *buf)
> {
This should be inlined into arch_hibernation_header_restore().
> 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;
There's no need to handle the "no digest" case anymore, right? Since crc32_le()
is always available.
- Eric
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 08, 2021 at 11:53:59AM -0400, Chris von Recklinghausen wrote:
> On 4/8/21 11:30 AM, Eric Biggers wrote:
> > On Thu, Apr 08, 2021 at 09:15:06AM -0400, 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 and is not intended as a
> > > cryptographic integrity check.
> > > 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)
> > Should CONFIG_CRYPTO_CRC32 be getting selected from somewhere?
>
>
> Yes, presumably from the same source that sets CONFIG_CRYPTO_MD5. Also
> presumably there's value to not forcing the check if the config value is not
> set.
I wouldn't be so sure about that. It might just be a bug that CONFIG_CRYPTO_MD5
wasn't being selected. Where is it documented that the user needed to set
CONFIG_CRYPTO_MD5=y if they wanted the hibernation image checksumming to work?
> >
> > If that is too hard because it would pull in too much of the crypto API,
> > maybe
> > using the library interface to CRC-32 (lib/crc32.c) would be a better fit?
>
>
> Based on my statement above, the intent is to provide a simple drop in
> replacement for md5 so that users of FIPS mode can suspend/resume without
> any errors.
>
It's possible that most people have CONFIG_CRYPTO_MD5 enabled for some unrelated
reason so the hibernation image checksumming works by chance.
CONFIG_CRYPTO_CRC32 is a different option so the same is not necessarily true.
However, note that CONFIG_HIBERNATION already selects CONFIG_CRC32 (the library
interface to CRC-32) but not CONFIG_CRYPTO_CRC32 (shash interface to CRC-32).
So, if this code just used the library interface crc32_le(), it will always be
available and the IS_BUILTIN() checks can be removed...
- Eric
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 08, 2021 at 09:15:06AM -0400, 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 and is not intended as a
> cryptographic integrity check.
> 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)
Should CONFIG_CRYPTO_CRC32 be getting selected from somewhere?
If that is too hard because it would pull in too much of the crypto API, maybe
using the library interface to CRC-32 (lib/crc32.c) would be a better fit?
- Eric
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
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...), then a non-cryptographic checksum would be sufficient.
- Eric
Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
On Wed, Apr 07, 2021 at 08:18:45PM +0530, Shreeya Patel wrote: > diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > index 2c27b9a5cd6c..0c69800a2a37 100644 > --- a/fs/unicode/Kconfig > +++ b/fs/unicode/Kconfig > @@ -2,13 +2,31 @@ > # > # UTF-8 normalization > # > +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 > +# is enabled. This config option adds the unicode subsystem layer which loads > +# the UTF-8 module whenever any filesystem needs it. > config UNICODE > - bool "UTF-8 normalization and casefolding support" > + bool > + > +config UNICODE_UTF8 > + tristate "UTF-8 module" > + select UNICODE > help > - Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding > - support. > + Say M here to enable UTF-8 NFD normalization and NFD+CF casefolding > + support as a loadable module or say Y for building it into the kernel. > + > + utf8data.h_shipped has a large database table which is an > + auto-generated decodification trie for the unicode normalization > + functions and it is not necessary to carry this large table in the > + kernel. Hence, enabling UNICODE_UTF8 as M will allow you to avoid > + carrying this large table into the kernel and module will only be > + loaded whenever required by any filesystem. > + Please note, in this case utf8 module will only be available after > + booting into the compiled kernel. If your filesystem requires it to > + have utf8 during boot time then you should have it built into the > + kernel by saying Y here to avoid boot failure. This help text seems to contradict itself; it says "it is not necessary to carry this large table in the kernel", and then later it says that in some cases it is in fact necessary. It would also be helpful for the help text to mention which filesystems actually support this feature. > diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c > index 730dbaedf593..d9e9e410893d 100644 > --- a/fs/unicode/unicode-core.c > +++ b/fs/unicode/unicode-core.c > @@ -1,228 +1,132 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > #include > #include > -#include > #include > -#include > #include > #include > -#include > +#include > > -#include "utf8n.h" > +DEFINE_SPINLOCK(utf8mod_lock); This spinlock should be 'static'. - Eric
Re: [PATCH v7 1/4] fs: unicode: Use strscpy() instead of strncpy()
On Wed, Apr 07, 2021 at 08:18:42PM +0530, Shreeya Patel wrote: > The -Wstringop-truncation warning highlights the unintended > uses of the strncpy function that truncate the terminating NULL > character from the source string. > Unlike strncpy(), strscpy() always null-terminates the destination string, > hence use strscpy() instead of strncpy(). This explanation is a bit misleading. It would be clearer to phrase this in terms of fixing how overly-long strings are handled: start returning an error instead of creating a non-null-terminated string. > > Fixes: 9d53690f0d4e5 (unicode: implement higher level API for string handling) There should be quotes around the commit title here. - Eric
Re: [PATCH v3 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Wed, Apr 07, 2021 at 06:04:21AM -0400, 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.
>
> Prior to this patch, MD5 is used only to create a digest to ensure integrity
> of
> the region, no actual encryption is done. This patch set changes the integrity
> check to use crc32 instead of md5 since crc32 is available in both FIPS and
> non-FIPS modes.
>
> 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
Please include an explanation about whether this use case requires cryptographic
security or not.
- Eric
Re: [PATCH] crypto: fix CRYPTO_LIB_* dependencies on CRYPTO
On Mon, Apr 05, 2021 at 11:04:38AM -0400, Julian Braha wrote: > Currently, when a config option selects a > CRYPTO_LIB_* option while CRYPTO is disabled, > Kbuild gives an unmet dependency. However, > these config options do not actually need to > depend on CRYPTO. > > Signed-off-by: Julian Braha > --- > crypto/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/crypto/Kconfig b/crypto/Kconfig > index 5809cc198fa7..fb7eca5cb8c6 100644 > --- a/crypto/Kconfig > +++ b/crypto/Kconfig > @@ -1870,9 +1870,9 @@ config CRYPTO_STATS > config CRYPTO_HASH_INFO > bool > > -source "lib/crypto/Kconfig" > source "drivers/crypto/Kconfig" > source "crypto/asymmetric_keys/Kconfig" > source "certs/Kconfig" > > endif# if CRYPTO > +source "lib/crypto/Kconfig" > -- Actually some of the files in lib/crypto/ do depend on CRYPTO. For example, there are calls to crypto_xor_cpy() and crypto_memneq(), which call functions defined in crypto/algapi.c and crypto/memneq.c. These helper functions would need to be moved into lib/crypto/ for this to work. - Eric
Re: [PATCH v6 4/4] fs: unicode: Add utf8 module and a unicode layer
On Thu, Apr 01, 2021 at 02:37:51AM +0530, Shreeya Patel wrote:
> +# utf8data.h_shipped has a large database table which is an auto-generated
> +# decodification trie for the unicode normalization functions and it is not
> +# necessary to carry this large table in the kernel.
> +# Enabling UNICODE_UTF8 option will allow UTF-8 encoding to be built as a
> +# module and this module will be loaded by the unicode subsystem layer only
> +# when any filesystem needs it.
> +config UNICODE_UTF8
> + tristate "UTF-8 module"
> help
> Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding
> support.
Please update this help text to properly describe this option, especially the
consequences of setting it to 'm'.
> + select UNICODE
'select' should go before 'help'.
> struct unicode_map *unicode_load(const char *version)
> {
> + try_then_request_module(utf8mod, "utf8");
> + if (!utf8mod) {
> + pr_err("Failed to load UTF-8 module\n");
> + return ERR_PTR(-ENODEV);
> }
>
> + spin_lock(_lock);
> + if (!utf8mod || !try_module_get(utf8mod)) {
> + spin_unlock(_lock);
> + return ERR_PTR(-ENODEV);
> + }
> + spin_unlock(_lock);
> + return static_call(unicode_load_static_call)(version);
> }
> EXPORT_SYMBOL(unicode_load);
>
> void unicode_unload(struct unicode_map *um)
> {
> kfree(um);
> +
> + spin_lock(_lock);
> + if (utf8mod)
> + module_put(utf8mod);
> + spin_unlock(_lock);
> +
> }
> EXPORT_SYMBOL(unicode_unload);
>
> +void unicode_register(struct module *owner)
> +{
> + utf8mod = owner;
> +}
> +EXPORT_SYMBOL(unicode_register);
> +
> +void unicode_unregister(void)
> +{
> + spin_lock(_lock);
> + utf8mod = NULL;
> + spin_unlock(_lock);
> +}
> +EXPORT_SYMBOL(unicode_unregister);
This all looks very broken. First, when !CONFIG_MODULES, utf8mod will always be
NULL so unicode_load() will always fail.
Also, if the unicode_load_static_call() fails, a reference to the utf8mod will
be leaked.
Also, unicode_unload() can put a reference to the utf8mod that was never
acquired.
Also there is a data race on utf8mod because the accesses to it aren't properly
synchronized.
Please consider something like the following, which I think would address all
these bugs:
static bool utf8mod_get(void)
{
bool ret;
spin_lock(_lock);
ret = utf8mod_loaded && try_module_get(utf8mod);
spin_unlock(_lock);
return ret;
}
struct unicode_map *unicode_load(const char *version)
{
struct unicode_map *um;
if (!try_then_request_module(utf8mod_get(), "utf8")) {
pr_err("Failed to load UTF-8 module\n");
return ERR_PTR(-ENODEV);
}
um = static_call(unicode_load_static_call)(version);
if (IS_ERR(um))
module_put(utf8mod);
return um;
}
EXPORT_SYMBOL(unicode_load);
void unicode_unload(struct unicode_map *um)
{
if (um) {
kfree(um);
module_put(utf8mod);
}
}
EXPORT_SYMBOL(unicode_unload);
void unicode_register(struct module *owner)
{
spin_lock(_lock);
utf8mod = owner; /* note: will be NULL if !CONFIG_MODULES */
utf8mod_loaded = true;
spin_unlock(_lock);
}
EXPORT_SYMBOL(unicode_register);
void unicode_unregister(void)
{
spin_lock(_lock);
utf8mod = NULL;
utf8mod_loaded = false;
spin_unlock(_lock);
}
EXPORT_SYMBOL(unicode_unregister);
Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 01, 2021 at 06:19:57PM +0200, Rafael J. Wysocki wrote:
> 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.
Collisions can easily be found for MD5 as well, as it is heavily broken.
Either you need a cryptographic hash function, *or* a (non-cryptographic)
checksum would be sufficient. There isn't really any in-between.
And if a checksum suffices, MD5 is a bad choice because it was designed as a
cryptographic hash function, so it is much slower than a checksum.
>
> 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 ...
If the 1 in 2^32 chance of a CRC-32 collision is too high, then use CRC-64 or
xxHash64 for a 1 in 2^64 chance of a collision.
- Eric
Re: Fix hibernation in FIPS mode?
On Thu, Apr 01, 2021 at 09:54:21AM -0400, Chris von Recklinghausen wrote:
> On 4/1/21 9:38 AM, 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.
>
>
> Would ghash be a better choice? It produces the same size digest as md5.
>
> Does anyone have any other suggestions of algorithms to try?
>
> Thanks,
>
> Chris
>
> >
> > > > 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.
> >
If you need to detect intentional changes (ensure authenticity, not just
integrity) then you need a cryptographic MAC, such as HMAC-SHA256.
If you only need to detect accidental changes (ensure integrity-only), then a
checksum such as CRC-32 or xxHash64 is sufficient. A cryptographic hash
function such as SHA-256 would also be sufficient, though much slower. Using a
broken cryptographic hash function such as MD5 doesn't make sense because it is
broken (so doesn't actually provide cryptographic security), and is much slower
than a checksum.
If the 1 in 4 billion collision rate of a CRC-32 isn't sufficient, then use
CRC-64 or xxHash64 for a 1 in 2^64 collision rate.
Don't use GHASH, as it is neither a checksum nor a cryptographic hash function.
- Eric
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Thu, Apr 01, 2021 at 08:50:05AM +0300, Jarkko Sakkinen wrote: > On Thu, Apr 01, 2021 at 12:11:32PM +1100, Herbert Xu wrote: > > On Wed, Mar 31, 2021 at 04:34:29PM -0700, Eric Biggers wrote: > > > On Thu, Apr 01, 2021 at 02:31:46AM +0300, Jarkko Sakkinen wrote: > > > > > > > > It's a bummer but uapi is the god in the end. Since TPM does not do it > > > > today, that behaviour must be supported forever. That's why a boot > > > > option > > > > AND a warning would be the best compromise. > > > > > > It's not UAPI if there is no way for userspace to tell if it changed. > > > > Exactly. UAPI is only an issue if something *breaks*. > > If there's even one user that comes shouting that he has a user space > configuration, where e.g. rng entropy is consumed constantly and the > code assumes that trusted keys does not add to that, then something > would break. > > It would be a crap user space yes, but I don't want to go on reverting > because of that. I think there is small but still existing chance that > something could break. random.c no longer provides any interfaces that subtract entropy credits, as that was never something that made sense. So "consuming" all the entropy from random.c isn't a thing anymore. > > Why not just add a boot parameter instead of making brutal enforcing > changes, indirectly visible to the user space? Why not just fix this bug instead of providing an option to fix it that everyone will need to remember to provide? - Eric
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Thu, Apr 01, 2021 at 02:31:46AM +0300, Jarkko Sakkinen wrote: > > It's a bummer but uapi is the god in the end. Since TPM does not do it > today, that behaviour must be supported forever. That's why a boot option > AND a warning would be the best compromise. > It's not UAPI if there is no way for userspace to tell if it changed. - Eric
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
On Tue, Mar 30, 2021 at 09:38:55AM -0700, Randy Dunlap wrote:
> On 3/29/21 10:29 PM, Eric Biggers wrote:
> > On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> >> Having just seen a report of using "fips=1" on the kernel command line,
> >> I could not find it documented anywhere, so add some help for it.
> >>
> >> Signed-off-by: Randy Dunlap
> >> Cc: Dexuan Cui
> >> Cc: linux-cry...@vger.kernel.org
> >> Cc: Eric Biggers
> >> Cc: Herbert Xu
> >> Cc: "David S. Miller"
> >> Cc: Jonathan Corbet
> >> Cc: linux-...@vger.kernel.org
> >> ---
> >> Updates/corrections welcome.
> >>
> >> v2: drop comment that "fips_enabled can cause some tests to be skipped".
> >>
> >> Documentation/admin-guide/kernel-parameters.txt | 14 ++
> >> 1 file changed, 14 insertions(+)
> >>
> >> ---
> >> linux-next-20210329.orig/Documentation/admin-guide/kernel-parameters.txt
> >> +++ linux-next-20210329/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1370,6 +1370,20 @@
> >>See Documentation/admin-guide/sysctl/net.rst for
> >>fb_tunnels_only_for_init_ns
> >>
> >> + fips= Format: { 0 | 1}
> >> + Use to disable (0) or enable (1) FIPS mode.
> >> + If enabled, any process that is waiting on the
> >> + 'fips_fail_notif_chain' will be notified of fips
> >> + failures.
> >> + This setting can also be modified via sysctl at
> >> + /proc/sysctl/crypto/fips_enabled, i.e.,
> >> + crypto.fips_enabled.
> >> + If fips_enabled = 1 and a test fails, it will cause a
> >> + kernel panic.
> >> + If fips_enabled = 1, RSA test requires a key size of
> >> + 2K or larger.
> >> + It can also effect which ECC curve is used.
> >
> > This doesn't really explain why anyone would want to give this option.
> > What high-level thing is this option meant to be accomplishing?
> > That's what the documentation should explain.
>
> Yes, clearly, even to me.
>
> But I could not find anything in the kernel source tree that would help me
> explain that. So to repeat:
>
> >> Updates/corrections welcome.
>
> thanks.
> --
I'm by no means an expert on this, but the main thing I have in mind is that
(IIUC) the "fips" option is only useful if your whole kernel binary is certified
as a "FIPS cryptographic module", *and* you actually need the FIPS compliance.
And the upstream kernel doesn't have a FIPS certification out of the box; that's
a task for specific Linux distributors like Red Hat, SUSE, Ubuntu, who get
specific kernel binaries certified.
So, compiling a kernel and using the "fips" option is useless by itself, as your
kernel image won't actually have a FIPS certification in that case anyway.
So, I would expect an explanation like that about under what circumstances the
"fips" option is actually useful and intended for.
The people who actually use this option should be able to explain it properly
though; the above is just my understanding...
- Eric
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Sun, Mar 28, 2021 at 11:37:23PM +0300, Jarkko Sakkinen wrote: > > Unfortunately, TPM trusted keys started this bad security practice, and > obviously it cannot be fixed without breaking uapi backwards compatibility. > The whole point of a randomness source is that it is random. So userspace can't be depending on any particular output, and the randomness source can be changed without breaking backwards compatibility. So IMO, trusted keys should simply be fixed to use get_random_bytes(). - Eric
Re: [PATCH v5 4/4] fs: unicode: Add utf8 module and a unicode layer
On Mon, Mar 29, 2021 at 10:16:57PM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers writes: > > > On Tue, Mar 30, 2021 at 02:12:40AM +0530, Shreeya Patel wrote: > >> diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > >> index 2c27b9a5cd6c..ad4b837f2eb2 100644 > >> --- a/fs/unicode/Kconfig > >> +++ b/fs/unicode/Kconfig > >> @@ -2,13 +2,26 @@ > >> # > >> # UTF-8 normalization > >> # > >> +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 > >> +# is enabled. This config option adds the unicode subsystem layer which > >> loads > >> +# the UTF-8 module whenever any filesystem needs it. > >> config UNICODE > >> - bool "UTF-8 normalization and casefolding support" > >> + bool > >> + > >> +# utf8data.h_shipped has a large database table which is an auto-generated > >> +# decodification trie for the unicode normalization functions and it is > >> not > >> +# necessary to carry this large table in the kernel. > >> +# Enabling UNICODE_UTF8 option will allow UTF-8 encoding to be built as a > >> +# module and this module will be loaded by the unicode subsystem layer > >> only > >> +# when any filesystem needs it. > >> +config UNICODE_UTF8 > >> + tristate "UTF-8 module" > >>help > >> Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding > >> support. > >> + select UNICODE > > > > This seems problematic; it allows users to set CONFIG_EXT4_FS=y (or > > CONFIG_F2FS_FS=y) but then CONFIG_UNICODE_UTF8=m. Then the filesystem won't > > work if the modules are located on the filesystem itself. > > Hi Eric, > > Isn't this a user problem? If the modules required to boot are on the > filesystem itself, you are in trouble. But, if that is the case, your > rootfs is case-insensitive and you gotta have utf8 as built-in or have > it in an early userspace. > We could make it the user's problem, but that seems rather unfriendly. Especially because the utf8 module would be needed if the filesystem has the casefold feature at all, regardless of whether any casefolded directories are needed at boot time or not. (Unless there is a plan to change that?) - Eric
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> Having just seen a report of using "fips=1" on the kernel command line,
> I could not find it documented anywhere, so add some help for it.
>
> Signed-off-by: Randy Dunlap
> Cc: Dexuan Cui
> Cc: linux-cry...@vger.kernel.org
> Cc: Eric Biggers
> Cc: Herbert Xu
> Cc: "David S. Miller"
> Cc: Jonathan Corbet
> Cc: linux-...@vger.kernel.org
> ---
> Updates/corrections welcome.
>
> v2: drop comment that "fips_enabled can cause some tests to be skipped".
>
> Documentation/admin-guide/kernel-parameters.txt | 14 ++
> 1 file changed, 14 insertions(+)
>
> --- linux-next-20210329.orig/Documentation/admin-guide/kernel-parameters.txt
> +++ linux-next-20210329/Documentation/admin-guide/kernel-parameters.txt
> @@ -1370,6 +1370,20 @@
> See Documentation/admin-guide/sysctl/net.rst for
> fb_tunnels_only_for_init_ns
>
> + fips= Format: { 0 | 1}
> + Use to disable (0) or enable (1) FIPS mode.
> + If enabled, any process that is waiting on the
> + 'fips_fail_notif_chain' will be notified of fips
> + failures.
> + This setting can also be modified via sysctl at
> + /proc/sysctl/crypto/fips_enabled, i.e.,
> + crypto.fips_enabled.
> + If fips_enabled = 1 and a test fails, it will cause a
> + kernel panic.
> + If fips_enabled = 1, RSA test requires a key size of
> + 2K or larger.
> + It can also effect which ECC curve is used.
This doesn't really explain why anyone would want to give this option.
What high-level thing is this option meant to be accomplishing?
That's what the documentation should explain.
- Eric
Re: [PATCH v5 4/4] fs: unicode: Add utf8 module and a unicode layer
On Tue, Mar 30, 2021 at 02:12:40AM +0530, Shreeya Patel wrote: > diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > index 2c27b9a5cd6c..ad4b837f2eb2 100644 > --- a/fs/unicode/Kconfig > +++ b/fs/unicode/Kconfig > @@ -2,13 +2,26 @@ > # > # UTF-8 normalization > # > +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 > +# is enabled. This config option adds the unicode subsystem layer which loads > +# the UTF-8 module whenever any filesystem needs it. > config UNICODE > - bool "UTF-8 normalization and casefolding support" > + bool > + > +# utf8data.h_shipped has a large database table which is an auto-generated > +# decodification trie for the unicode normalization functions and it is not > +# necessary to carry this large table in the kernel. > +# Enabling UNICODE_UTF8 option will allow UTF-8 encoding to be built as a > +# module and this module will be loaded by the unicode subsystem layer only > +# when any filesystem needs it. > +config UNICODE_UTF8 > + tristate "UTF-8 module" > help > Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding > support. > + select UNICODE This seems problematic; it allows users to set CONFIG_EXT4_FS=y (or CONFIG_F2FS_FS=y) but then CONFIG_UNICODE_UTF8=m. Then the filesystem won't work if the modules are located on the filesystem itself. I think it should work analogously to CONFIG_FS_ENCRYPTION and CONFIG_FS_ENCRYPTION_ALGS. That is, CONFIG_UNICODE should be a user-selectable bool, and then the tristate symbols CONFIG_EXT4_FS and CONFIG_F2FS_FS should select the tristate symbol CONFIG_UNICODE_UTF8 if CONFIG_UNICODE. - Eric
Re: [PATCH v5 2/4] fs: unicode: Rename function names from utf8 to unicode
On Tue, Mar 30, 2021 at 02:12:38AM +0530, Shreeya Patel wrote: > utf8data.h_shipped has a large database table which is an auto-generated > decodification trie for the unicode normalization functions and it is not > necessary to carry this large table in the kernel. > Goal is to make UTF-8 encoding loadable by converting it into a module > and adding a unicode subsystem layer between the filesystems and the > utf8 module. > This layer will load the module whenever any filesystem that > needs unicode is mounted. > utf8-core will be converted into this layer file in the future patches, > hence rename the function names from utf8 to unicode which will denote the > functions as the unicode subsystem layer functions and this will also be > the first step towards the transformation of utf8-core file into the > unicode subsystem layer file. > > Signed-off-by: Shreeya Patel > --- > Changes in v5 > - Improve the commit message. This didn't really answer my questions about the reason for this renaming. Aren't the functions like unicode_casefold() still tied to UTF-8 (as opposed to e.g. supporting both UTF-8 and UTF-16)? Is that something you're trying to change? - Eric
Re: [PATCH 1/3] fs/dcache: Add d_clear_dir_neg_dentries()
On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:
> For directories with negative dentries that are becoming case-insensitive
> dirs, we need to remove all those negative dentries, otherwise they will
> become dangling dentries. During the creation of a new file, if a d_hash
> collision happens and the names match in a case-insensitive way, the name
> of the file will be the name defined at the negative dentry, that may be
> different from the specified by the user. To prevent this from
> happening, we need to remove all dentries in a directory. Given that the
> directory must be empty before we call this function we are sure that
> all dentries there will be negative.
>
> Create a function to remove all negative dentries from a directory, to
> be used as explained above by filesystems that support case-insensitive
> lookups.
>
> Signed-off-by: André Almeida
> ---
> fs/dcache.c| 27 +++
> include/linux/dcache.h | 1 +
> 2 files changed, 28 insertions(+)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7d24ff7eb206..fafb3016d6fd 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1723,6 +1723,33 @@ void d_invalidate(struct dentry *dentry)
> }
> EXPORT_SYMBOL(d_invalidate);
>
> +/**
> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
> + * @dir: Directory to clear negative dentries
> + *
> + * For directories with negative dentries that are becoming case-insensitive
> + * dirs, we need to remove all those negative dentries, otherwise they will
> + * become dangling dentries. During the creation of a new file, if a d_hash
> + * collision happens and the names match in a case-insensitive, the name of
> + * the file will be the name defined at the negative dentry, that can be
> + * different from the specified by the user. To prevent this from happening,
> we
> + * need to remove all dentries in a directory. Given that the directory must
> be
> + * empty before we call this function we are sure that all dentries there
> will
> + * be negative.
> + */
> +void d_clear_dir_neg_dentries(struct inode *dir)
> +{
> + struct dentry *alias, *dentry;
> +
> + hlist_for_each_entry(alias, >i_dentry, d_u.d_alias) {
> + list_for_each_entry(dentry, >d_subdirs, d_child) {
> + d_drop(dentry);
> + dput(dentry);
> + }
> + }
> +}
> +EXPORT_SYMBOL(d_clear_dir_neg_dentries);
As Al already pointed out, this doesn't work as intended, for a number of
different reasons.
Did you consider just using shrink_dcache_parent()? That already does what you
are trying to do here, I think.
The harder part (which I don't think you've considered) is how to ensure that
all negative dentries really get invalidated even if there are lookups of them
happening concurrently. Concurrent lookups can take temporary references to the
negative dentries, preventing them from being invalidated.
- Eric
Re: v5.12.0-rc5: the kernel panics if FIPS mode is on
On Mon, Mar 29, 2021 at 09:56:18PM +, Dexuan Cui wrote: > Hi all, > The v5.12.0-rc5 kernel (1e43c377a79f) panics with fips=1. > > Please refer to the below panic call-trace. The kernel config file and > the full kernel messages are also attached. > > Is this a known issue? > > Thanks, > -- Dexuan > > Starting dracut pre-udev hook... > [7.260424] alg: self-tests for sha512-generic (sha512) passed > [7.265917] alg: self-tests for sha384-generic (sha384) passed > [7.272426] alg: self-tests for sha512-ssse3 (sha512) passed > [7.276500] alg: self-tests for sha384-ssse3 (sha384) passed > [7.281722] alg: self-tests for sha512-avx (sha512) passed > [7.286579] alg: self-tests for sha384-avx (sha384) passed > [7.291631] alg: self-tests for sha512-avx2 (sha512) passed > [7.296950] alg: self-tests for sha384-avx2 (sha384) passed > [7.321040] alg: self-tests for sha3-224-generic (sha3-224) passed > [7.330291] alg: self-tests for sha3-256-generic (sha3-256) passed > [7.335918] alg: self-tests for sha3-384-generic (sha3-384) passed > [7.341508] alg: self-tests for sha3-512-generic (sha3-512) passed > [7.381918] alg: self-tests for crc32c-intel (crc32c) passed > [7.396694] alg: self-tests for crct10dif-pclmul (crct10dif) passed > [7.453515] alg: self-tests for ghash-clmulni (ghash) passed > [7.469558] alg: self-tests for des3_ede-asm (des3_ede) passed > [7.475355] alg: self-tests for ecb-des3_ede-asm (ecb(des3_ede)) passed > [7.481361] alg: self-tests for cbc-des3_ede-asm (cbc(des3_ede)) passed > [7.488656] alg: self-tests for des3_ede-generic (des3_ede) passed > [7.304930] dracut-pre-udev[502]: modprobe: ERROR: could not insert > 'padlock_aes': No such device > [7.579580] alg: No test for fips(ansi_cprng) (fips_ansi_cprng) > [7.606547] alg: self-tests for sha1 (sha1) passed > [7.610624] alg: self-tests for ecb(des3_ede) (ecb(des3_ede)) passed > [7.615746] alg: self-tests for cbc(des3_ede) (cbc(des3_ede)) passed > [7.638067] alg: self-tests for ctr(des3_ede-asm) (ctr(des3_ede)) passed > [7.644781] alg: self-tests for ctr(des3_ede) (ctr(des3_ede)) passed > [7.653810] alg: self-tests for sha256 (sha256) passed > [7.658945] alg: self-tests for ecb(aes) (ecb(aes)) passed > [7.663493] alg: self-tests for cbc(aes) (cbc(aes)) passed > [7.668421] alg: self-tests for xts(aes) (xts(aes)) passed > [7.672389] alg: self-tests for ctr(aes) (ctr(aes)) passed > [7.692973] alg: self-tests for rfc3686(ctr-aes-aesni) (rfc3686(ctr(aes))) > passed > [7.699446] alg: self-tests for rfc3686(ctr(aes)) (rfc3686(ctr(aes))) > passed > [7.730149] alg: skcipher: failed to allocate transform for ofb(aes): -2 > [7.735959] Kernel panic - not syncing: alg: self-tests for ofb(aes) > (ofb(aes)) failed in fips mode! > [7.736952] CPU: 13 PID: 560 Comm: modprobe Tainted: GW > 5.12.0-rc5+ #3 > [7.736952] Hardware name: Microsoft Corporation Virtual Machine/Virtual > Machine, BIOS 090008 12/07/2018 > [7.736952] Call Trace: > [7.736952] dump_stack+0x64/0x7c > [7.736952] panic+0xfb/0x2d7 > [7.736952] alg_test+0x42d/0x460 > [7.736952] ? __kernfs_new_node+0x175/0x1d0 > [7.736952] do_test+0x3248/0x57ea [tcrypt] > [7.736952] do_test+0x1f2c/0x57ea [tcrypt] > [7.736952] ? 0xc031d000 > [7.736952] tcrypt_mod_init+0x55/0x1000 [tcrypt] It looks like your userspace is using tcrypt.ko to request that the kernel test "ofb(aes)", but your kernel doesn't have CONFIG_CRYPTO_OFB enabled so the test fails as expected. Are you sure that anything changed on the kernel side besides the kconfig you are using? It looks like this was always the behavior when tcrypt.ko is used to test a non-existing algorithm. Is your userspace code intentionally trying to test "ofb(aes)", or is it accidental? - Eric
Re: [PATCH v4 2/5] fs: Check if utf8 encoding is loaded before calling utf8_unload()
On Thu, Mar 25, 2021 at 03:31:42PM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers writes: > > > On Thu, Mar 25, 2021 at 05:38:08AM +0530, Shreeya Patel wrote: > >> utf8_unload is being called if CONFIG_UNICODE is enabled. > >> The ifdef block doesn't check if utf8 encoding has been loaded > >> or not before calling the utf8_unload() function. > >> This is not the expected behavior since it would sometimes lead > >> to unloading utf8 even before loading it. > >> Hence, add a condition which will check if sb->encoding is NOT NULL > >> before calling the utf8_unload(). > >> > >> Reviewed-by: Gabriel Krisman Bertazi > >> Signed-off-by: Shreeya Patel > >> --- > >> fs/ext4/super.c | 6 -- > >> fs/f2fs/super.c | 9 ++--- > >> 2 files changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c > >> index ad34a37278cd..e438d14f9a87 100644 > >> --- a/fs/ext4/super.c > >> +++ b/fs/ext4/super.c > >> @@ -1259,7 +1259,8 @@ static void ext4_put_super(struct super_block *sb) > >>fs_put_dax(sbi->s_daxdev); > >>fscrypt_free_dummy_policy(>s_dummy_enc_policy); > >> #ifdef CONFIG_UNICODE > >> - utf8_unload(sb->s_encoding); > >> + if (sb->s_encoding) > >> + utf8_unload(sb->s_encoding); > >> #endif > >>kfree(sbi); > >> } > > > > > > What's the benefit of this change? utf8_unload is a no-op when passed a > > NULL > > pointer; why not keep it that way? > > For the record, it no longer is a no-op after patch 5 of this series. > Honestly, I prefer making it explicitly at the caller that we are not > entering the function, like the patch does, instead of returning from it > immediately. Makes it more readable, IMO. > I don't think making all the callers do the NULL check is more readable. It's conventional for free-like functions to accept NULL pointers. See for example every other function in the code snippet above -- fs_put_dax(), fscrypt_free_dummy_policy(), and kfree(). This seems more like an issue with patch 5; it shouldn't be dropping the NULL check from unicode_unload(). - Eric
Re: [PATCH v4 5/5] fs: unicode: Add utf8 module and a unicode layer
On Thu, Mar 25, 2021 at 05:38:11AM +0530, Shreeya Patel wrote: > Also, indirect calls using function pointers are easily exploitable by > speculative execution attacks, hence use static_call() in unicode.h and > unicode-core.c files inorder to prevent these attacks by making direct > calls and also to improve the performance of function pointers. I don't think you need to worry about avoiding indirect calls to prevent speculative execution attacks. That's what the mitigations like Retpoline are for. Instead my concern was just that indirect calls are *slow*, especially when those mitigations are enabled. Some of the casefolding operations are called a lot (e.g., repeatedly during path resolution), and it would be desirable to avoid adding more overhead there. > diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > index 2c27b9a5cd6c..2961b0206b4d 100644 > --- a/fs/unicode/Kconfig > +++ b/fs/unicode/Kconfig > @@ -8,7 +8,16 @@ config UNICODE > Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding > support. > > +# UTF-8 encoding can be compiled as a module using UNICODE_UTF8 option. > +# Having UTF-8 encoding as a module will avoid carrying large > +# database table present in utf8data.h_shipped into the kernel > +# by being able to load it only when it is required by the filesystem. > +config UNICODE_UTF8 > + tristate "UTF-8 module" > + depends on UNICODE > + default m > + The help for UNICODE still says that it enables UTF-8 support. But now there is a separate option that people will need to remember to enable. Please document each of these options properly. Perhaps EXT4_FS and F2FS_FS just should select UNICODE_UTF8 if UNICODE, so that UNICODE_UTF8 doesn't have to be a user-selectable symbol? > +DEFINE_STATIC_CALL(validate, unicode_validate_static_call); > +EXPORT_STATIC_CALL(validate); Global symbols can't have generic names like "validate". Please add an appropriate prefix like "unicode_". Also, the thing called "unicode_validate_static_call" isn't actually a static call as the name suggests, but rather the default function used by the static call. It should be called something like unicode_validate_default. Likewise for all the others. - Eric
Re: [PATCH v4 3/5] fs: unicode: Rename function names from utf8 to unicode
On Thu, Mar 25, 2021 at 05:38:09AM +0530, Shreeya Patel wrote: > Rename the function names from utf8 to unicode for taking the first step > towards the transformation of utf8-core file into the unicode subsystem > layer file. > > Reviewed-by: Gabriel Krisman Bertazi > Signed-off-by: Shreeya Patel Can you add some more explanation about why this change is beneficial? The functions still seem tied to UTF-8 specifically. - Eric
Re: [PATCH v4 2/5] fs: Check if utf8 encoding is loaded before calling utf8_unload()
On Thu, Mar 25, 2021 at 05:38:08AM +0530, Shreeya Patel wrote: > utf8_unload is being called if CONFIG_UNICODE is enabled. > The ifdef block doesn't check if utf8 encoding has been loaded > or not before calling the utf8_unload() function. > This is not the expected behavior since it would sometimes lead > to unloading utf8 even before loading it. > Hence, add a condition which will check if sb->encoding is NOT NULL > before calling the utf8_unload(). > > Reviewed-by: Gabriel Krisman Bertazi > Signed-off-by: Shreeya Patel > --- > fs/ext4/super.c | 6 -- > fs/f2fs/super.c | 9 ++--- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index ad34a37278cd..e438d14f9a87 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1259,7 +1259,8 @@ static void ext4_put_super(struct super_block *sb) > fs_put_dax(sbi->s_daxdev); > fscrypt_free_dummy_policy(>s_dummy_enc_policy); > #ifdef CONFIG_UNICODE > - utf8_unload(sb->s_encoding); > + if (sb->s_encoding) > + utf8_unload(sb->s_encoding); > #endif > kfree(sbi); > } What's the benefit of this change? utf8_unload is a no-op when passed a NULL pointer; why not keep it that way? - Eric
Re: [PATCH v3 5/5] fs: unicode: Add utf8 module and a unicode layer
On Tue, Mar 23, 2021 at 03:51:44PM -0400, Gabriel Krisman Bertazi wrote:
> > -int unicode_validate(const struct unicode_map *um, const struct qstr *str)
> > -{
> > - const struct utf8data *data = utf8nfdi(um->version);
> > -
> > - if (utf8nlen(data, str->name, str->len) < 0)
> > - return -1;
> > - return 0;
> > -}
> > +struct unicode_ops *utf8_ops;
> > +EXPORT_SYMBOL(utf8_ops);
> > +
> > +int _utf8_validate(const struct unicode_map *um, const struct qstr *str)
> > +{
> > + return 0;
> > +}
> > -EXPORT_SYMBOL(unicode_validate);
>
> I think that any calls to the default static calls should return errors
> instead of succeeding without doing anything.
>
> In fact, are the default calls really necessary? If someone gets here,
> there is a bug elsewhere, so WARN_ON and maybe -EIO.
>
> int unicode_validate_default_static_call(...)
> {
>WARN_ON(1);
>return -EIO;
> }
>
> Or just have a NULL default, as I mentioned below, if that is possible.
>
[...]
> > +DEFINE_STATIC_CALL(utf8_validate, _utf8_validate);
> > +DEFINE_STATIC_CALL(utf8_strncmp, _utf8_strncmp);
> > +DEFINE_STATIC_CALL(utf8_strncasecmp, _utf8_strncasecmp);
> > +DEFINE_STATIC_CALL(utf8_strncasecmp_folded, _utf8_strncasecmp_folded);
> > +DEFINE_STATIC_CALL(utf8_normalize, _utf8_normalize);
> > +DEFINE_STATIC_CALL(utf8_casefold, _utf8_casefold);
> > +DEFINE_STATIC_CALL(utf8_casefold_hash, _utf8_casefold_hash);
> > +DEFINE_STATIC_CALL(utf8_load, _utf8_load);
> > +DEFINE_STATIC_CALL_NULL(utf8_unload, _utf8_unload);
> > +EXPORT_STATIC_CALL(utf8_strncmp);
> > +EXPORT_STATIC_CALL(utf8_strncasecmp);
> > +EXPORT_STATIC_CALL(utf8_strncasecmp_folded);
>
> I'm having a hard time understanding why some use
> DEFINE_STATIC_CALL_NULL, while other use DEFINE_STATIC_CALL. This new
> static call API is new to me :). None of this can be called if the
> module is not loaded anyway, so perhaps the default function can just be
> NULL, per the documentation of include/linux/static_call.h?
>
> Anyway, Aren't utf8_{validate,casefold,normalize} missing the
> equivalent EXPORT_STATIC_CALL?
>
The static_call API is fairly new to me too. But the intent of this patch seems
to be that none of the utf8 functions are called without the utf8 module loaded.
If they are called, it's a kernel bug. So there are two options for what to do
if it happens anyway:
1. call a "null" static call, which does nothing
*or*
2. call a default function which does WARN_ON_ONCE() and returns an error if
possible.
(or 3. don't use static calls and instead dereference a NULL utf8_ops like
previous versions of this patch did.)
It shouldn't really matter which of these approaches you take, but please be
consistent and use the same one everywhere.
> + void unicode_unregister(void)
> + {
> + spin_lock(_lock);
> + utf8_ops = NULL;
> + spin_unlock(_lock);
> + }
> + EXPORT_SYMBOL(unicode_unregister);
This should restore the static calls to their default values (either NULL or the
default functions, depending on what you decide).
Also, it's weird to still have the utf8_ops structure when using static calls.
It seems it should be one way or the other: static calls *or* utf8_ops.
The static calls could be exported, and the module could be responsible for
updating them. That would eliminate the need for utf8_ops.
- Eric
Re: [PATCH] keys: Allow disabling read permissions for key possessor
On Mon, Mar 22, 2021 at 12:57:26PM +0300, Andrey Ryabinin wrote:
> keyctl_read_key() has a strange code which allows possessor to read
> key's payload regardless of READ permission status:
>
> $ keyctl add user test test @u
> 196773443
> $ keyctl print 196773443
> test
> $ keyctl describe 196773443
> 196773443: alswrv-v 1000 1000 user: test
> $ keyctl rdescribe 196773443
> user;1000;1000;3f01;test
> $ keyctl setperm 196773443 0x3d01
> $ keyctl describe 196773443
> 196773443: alsw-v-v 1000 1000 user: test
> $ keyctl print 196773443
> test
>
> The last keyctl print should fail with -EACCESS instead of success.
> Fix this by removing weird possessor checks.
>
> Signed-off-by: Andrey Ryabinin
> ---
>
> - This was noticed by code review. It seems like a bug to me,
> but if I'm wrong and current behavior is correct, I think we need
> at least better comment here.
>
>
> security/keys/keyctl.c | 15 +--
> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 96a92a645216d..2ec021c7adc12 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -845,22 +845,9 @@ long keyctl_read_key(key_serial_t keyid, char __user
> *buffer, size_t buflen)
>
> /* see if we can read it directly */
> ret = key_permission(key_ref, KEY_NEED_READ);
> - if (ret == 0)
> - goto can_read_key;
> - if (ret != -EACCES)
> + if (ret != 0)
> goto key_put_out;
>
> - /* we can't; see if it's searchable from this process's keyrings
> - * - we automatically take account of the fact that it may be
> - * dangling off an instantiation key
> - */
> - if (!is_key_possessed(key_ref)) {
> - ret = -EACCES;
> - goto key_put_out;
> - }
> -
This might be intentional, given the comment above the function:
* The key must either grant the caller Read permission, or it must grant the
* caller Search permission when searched for from the process keyrings.
The 'is_key_possessed()' check is implementing the second part, right?
Maybe check if this shows up in the documentation and tests too.
- Eric
Re: [PATCH] crypto: poly1305: fix poly1305_core_setkey() declaration
On Mon, Mar 22, 2021 at 07:51:47PM +0100, Ard Biesheuvel wrote:
> On Mon, 22 Mar 2021 at 18:05, Arnd Bergmann wrote:
> >
> > From: Arnd Bergmann
> >
> > gcc-11 points out a mismatch between the declaration and the definition
> > of poly1305_core_setkey():
> >
> > lib/crypto/poly1305-donna32.c:13:67: error: argument 2 of type ‘const
> > u8[16]’ {aka ‘const unsigned char[16]’} with mismatched bound
> > [-Werror=array-parameter=]
> >13 | void poly1305_core_setkey(struct poly1305_core_key *key, const u8
> > raw_key[16])
> > |
> > ~^~~
> > In file included from lib/crypto/poly1305-donna32.c:11:
> > include/crypto/internal/poly1305.h:21:68: note: previously declared as
> > ‘const u8 *’ {aka ‘const unsigned char *’}
> >21 | void poly1305_core_setkey(struct poly1305_core_key *key, const u8
> > *raw_key);
> >
> > This is harmless in principle, as the calling conventions are the same,
> > but the more specific prototype allows better type checking in the
> > caller.
> >
> > Change the declaration to match the actual function definition.
> > The poly1305_simd_init() is a bit suspicious here, as it previously
> > had a 32-byte argument type, but looks like it needs to take the
> > 16-byte POLY1305_BLOCK_SIZE array instead.
> >
>
> This looks ok to me. For historical reasons, the Poly1305 integration
> is based on an unkeyed shash, and both the Poly1305 key and nonce are
> passed as ordinary input, prepended to the actual data.
> poly1305_simd_init() takes only the key but not the nonce, so it
> should only be passed 16 bytes.
Well to be more precise, there are two conventions for using Poly1305. One
where it is invoked many times with the same 16-byte key and different 16-byte
nonces. And one where every invocation uses a unique key *and* nonce,
interpreted as a 32-byte "one-time key".
So that's why there's a mix of 16 and 32 byte "keys".
The naming "POLY1305_KEY_SIZE" assumes the second convention, which is a bit
confusing; it really should be called something like POLY1305_ONETIME_KEY_SIZE.
I guess the idea was that the one-time key convention is the more common one.
Anyway, the patch seems to be fine, as it uses the correct length in each
location. You can add:
Reviewed-by: Eric Biggers
- Eric
Re: [PATCH 07/18] f2fs: convert to miscattr
On Wed, Feb 03, 2021 at 01:41:01PM +0100, Miklos Szeredi wrote:
> @@ -3071,123 +3012,54 @@ static int f2fs_ioc_setproject(struct file *filp,
> __u32 projid)
> }
> #endif
>
> -/* FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR support */
> -
> -/*
> - * To make a new on-disk f2fs i_flag gettable via FS_IOC_FSGETXATTR and
> settable
> - * via FS_IOC_FSSETXATTR, add an entry for it to f2fs_xflags_map[], and add
> its
> - * FS_XFLAG_* equivalent to F2FS_SUPPORTED_XFLAGS.
> - */
> -
> -static const struct {
> - u32 iflag;
> - u32 xflag;
> -} f2fs_xflags_map[] = {
> - { F2FS_SYNC_FL, FS_XFLAG_SYNC },
> - { F2FS_IMMUTABLE_FL,FS_XFLAG_IMMUTABLE },
> - { F2FS_APPEND_FL, FS_XFLAG_APPEND },
> - { F2FS_NODUMP_FL, FS_XFLAG_NODUMP },
> - { F2FS_NOATIME_FL, FS_XFLAG_NOATIME },
> - { F2FS_PROJINHERIT_FL, FS_XFLAG_PROJINHERIT },
> -};
There's another comment just above which talks about FS_IOC_GETFLAGS and
FS_IOC_SETFLAGS:
/* FS_IOC_GETFLAGS and FS_IOC_SETFLAGS support */
/*
* To make a new on-disk f2fs i_flag gettable via FS_IOC_GETFLAGS, add
an entry
* for it to f2fs_fsflags_map[], and add its FS_*_FL equivalent to
* F2FS_GETTABLE_FS_FL. To also make it settable via FS_IOC_SETFLAGS,
also add
* its FS_*_FL equivalent to F2FS_SETTABLE_FS_FL.
*/
This patch seems to make that outdated, since now both FS_IOC_[GS]ETFLAGS and
FS_IOC_[GS]ETFSXATTR are handled together.
Can you please update the comment to properly describe what's going on?
- Eric
Re: [PATCH 01/18] vfs: add miscattr ops
On Wed, Feb 03, 2021 at 01:40:55PM +0100, Miklos Szeredi wrote:
> + * Verifying attributes involves retrieving current attributes with
> + * i_op->miscattr_get(), this also allows initilaizing attributes that have
initilaizing => initializing
> +int vfs_miscattr_set(struct dentry *dentry, struct miscattr *ma)
> +{
> + struct inode *inode = d_inode(dentry);
> + struct miscattr old_ma = {};
> + int err;
> +
> + if (d_is_special(dentry))
> + return -ENOTTY;
> +
> + if (!inode->i_op->miscattr_set)
> + return -ENOIOCTLCMD;
> +
> + if (!inode_owner_or_capable(inode))
> + return -EPERM;
Shouldn't this be EACCES, not EPERM?
> +/**
> + * miscattr_has_xattr - check for extentended flags/attributes
extentended => extended
- Eric
Re: [PATCH 2/2] integrity: double check iint_cache was initialized
On Mon, Mar 22, 2021 at 11:42:07AM -0400, Mimi Zohar wrote:
>
> Reported-by: Dmitry Vyukov
> Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
> Signed-off-by: Mimi Zohar
Missing Cc stable?
- Eric
Re: [PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded
On Mon, Mar 22, 2021 at 11:42:06AM -0400, Mimi Zohar wrote: > Only after an IMA policy is loaded, check, save, or update the cached > file's integrity status. > > Signed-off-by: Mimi Zohar This commit message doesn't describe what the actual effect of this change is. Is it fixing something? - Eric
[PATCH RESEND] random: remove dead code left over from blocking pool
From: Eric Biggers
Remove some dead code that was left over following commit 90ea1c6436d2
("random: remove the blocking pool").
Cc: linux-cry...@vger.kernel.org
Cc: Andy Lutomirski
Cc: Jann Horn
Cc: Theodore Ts'o
Reviewed-by: Andy Lutomirski
Acked-by: Ard Biesheuvel
Signed-off-by: Eric Biggers
---
drivers/char/random.c | 17 ++-
include/trace/events/random.h | 83 ---
2 files changed, 3 insertions(+), 97 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 5d6acfecd919b..605969ed0f965 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -500,7 +500,6 @@ struct entropy_store {
unsigned short add_ptr;
unsigned short input_rotate;
int entropy_count;
- unsigned int initialized:1;
unsigned int last_data_init:1;
__u8 last_data[EXTRACT_SIZE];
};
@@ -660,7 +659,7 @@ static void process_random_ready_list(void)
*/
static void credit_entropy_bits(struct entropy_store *r, int nbits)
{
- int entropy_count, orig, has_initialized = 0;
+ int entropy_count, orig;
const int pool_size = r->poolinfo->poolfracbits;
int nfrac = nbits << ENTROPY_SHIFT;
@@ -717,23 +716,14 @@ static void credit_entropy_bits(struct entropy_store *r,
int nbits)
if (cmpxchg(>entropy_count, orig, entropy_count) != orig)
goto retry;
- if (has_initialized) {
- r->initialized = 1;
- kill_fasync(, SIGIO, POLL_IN);
- }
-
trace_credit_entropy_bits(r->name, nbits,
entropy_count >> ENTROPY_SHIFT, _RET_IP_);
if (r == _pool) {
int entropy_bits = entropy_count >> ENTROPY_SHIFT;
- if (crng_init < 2) {
- if (entropy_bits < 128)
- return;
+ if (crng_init < 2 && entropy_bits >= 128)
crng_reseed(_crng, r);
- entropy_bits = ENTROPY_BITS(r);
- }
}
}
@@ -1372,8 +1362,7 @@ static size_t account(struct entropy_store *r, size_t
nbytes, int min,
}
/*
- * This function does the actual extraction for extract_entropy and
- * extract_entropy_user.
+ * This function does the actual extraction for extract_entropy.
*
* Note: we assume that .poolwords is a multiple of 16 words.
*/
diff --git a/include/trace/events/random.h b/include/trace/events/random.h
index 9570a10cb949b..3d7b432ca5f31 100644
--- a/include/trace/events/random.h
+++ b/include/trace/events/random.h
@@ -85,28 +85,6 @@ TRACE_EVENT(credit_entropy_bits,
__entry->entropy_count, (void *)__entry->IP)
);
-TRACE_EVENT(push_to_pool,
- TP_PROTO(const char *pool_name, int pool_bits, int input_bits),
-
- TP_ARGS(pool_name, pool_bits, input_bits),
-
- TP_STRUCT__entry(
- __field( const char *, pool_name )
- __field( int, pool_bits )
- __field( int, input_bits )
- ),
-
- TP_fast_assign(
- __entry->pool_name = pool_name;
- __entry->pool_bits = pool_bits;
- __entry->input_bits = input_bits;
- ),
-
- TP_printk("%s: pool_bits %d input_pool_bits %d",
- __entry->pool_name, __entry->pool_bits,
- __entry->input_bits)
-);
-
TRACE_EVENT(debit_entropy,
TP_PROTO(const char *pool_name, int debit_bits),
@@ -161,35 +139,6 @@ TRACE_EVENT(add_disk_randomness,
MINOR(__entry->dev), __entry->input_bits)
);
-TRACE_EVENT(xfer_secondary_pool,
- TP_PROTO(const char *pool_name, int xfer_bits, int request_bits,
-int pool_entropy, int input_entropy),
-
- TP_ARGS(pool_name, xfer_bits, request_bits, pool_entropy,
- input_entropy),
-
- TP_STRUCT__entry(
- __field( const char *, pool_name )
- __field( int, xfer_bits )
- __field( int, request_bits)
- __field( int, pool_entropy)
- __field( int, input_entropy )
- ),
-
- TP_fast_assign(
- __entry->pool_name = pool_name;
- __entry->xfer_bits = xfer_bits;
- __entry->request_bits = request_bits;
- __entry->pool_entropy = pool_entropy;
- __entry->input_entropy = input_entropy;
- ),
-
- TP_printk("pool %s xfer_bits %d request_bits %d pool_entropy %d "
- "input_entropy %d", __entry->pool_name, __entry->xfer_bits,
- __entry->request_bits, __entry->pool_entropy,
-
[PATCH RESEND] random: initialize ChaCha20 constants with correct endianness
From: Eric Biggers
On big endian CPUs, the ChaCha20-based CRNG is using the wrong
endianness for the ChaCha20 constants.
This doesn't matter cryptographically, but technically it means it's not
ChaCha20 anymore. Fix it to always use the standard constants.
Cc: linux-cry...@vger.kernel.org
Cc: Andy Lutomirski
Cc: Jann Horn
Cc: Theodore Ts'o
Acked-by: Herbert Xu
Acked-by: Ard Biesheuvel
Signed-off-by: Eric Biggers
---
drivers/char/random.c | 4 ++--
include/crypto/chacha.h | 9 +++--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0fe9e200e4c84..5d6acfecd919b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -819,7 +819,7 @@ static bool __init crng_init_try_arch_early(struct
crng_state *crng)
static void __maybe_unused crng_initialize_secondary(struct crng_state *crng)
{
- memcpy(>state[0], "expand 32-byte k", 16);
+ chacha_init_consts(crng->state);
_get_random_bytes(>state[4], sizeof(__u32) * 12);
crng_init_try_arch(crng);
crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
@@ -827,7 +827,7 @@ static void __maybe_unused crng_initialize_secondary(struct
crng_state *crng)
static void __init crng_initialize_primary(struct crng_state *crng)
{
- memcpy(>state[0], "expand 32-byte k", 16);
+ chacha_init_consts(crng->state);
_extract_entropy(_pool, >state[4], sizeof(__u32) * 12, 0);
if (crng_init_try_arch_early(crng) && trust_cpu) {
invalidate_batched_entropy();
diff --git a/include/crypto/chacha.h b/include/crypto/chacha.h
index 3a1c72fdb7cf5..dabaee6987186 100644
--- a/include/crypto/chacha.h
+++ b/include/crypto/chacha.h
@@ -47,13 +47,18 @@ static inline void hchacha_block(const u32 *state, u32
*out, int nrounds)
hchacha_block_generic(state, out, nrounds);
}
-void chacha_init_arch(u32 *state, const u32 *key, const u8 *iv);
-static inline void chacha_init_generic(u32 *state, const u32 *key, const u8
*iv)
+static inline void chacha_init_consts(u32 *state)
{
state[0] = 0x61707865; /* "expa" */
state[1] = 0x3320646e; /* "nd 3" */
state[2] = 0x79622d32; /* "2-by" */
state[3] = 0x6b206574; /* "te k" */
+}
+
+void chacha_init_arch(u32 *state, const u32 *key, const u8 *iv);
+static inline void chacha_init_generic(u32 *state, const u32 *key, const u8
*iv)
+{
+ chacha_init_consts(state);
state[4] = key[0];
state[5] = key[1];
state[6] = key[2];
--
2.31.0
Re: [PATCH v2 4/4] fs: unicode: Add utf8 module and a unicode layer
On Thu, Mar 18, 2021 at 07:03:05PM +0530, Shreeya Patel wrote:
> +struct unicode_ops {
> + struct module *owner;
> + int (*validate)(const struct unicode_map *um, const struct qstr *str);
> + int (*strncmp)(const struct unicode_map *um, const struct qstr *s1,
> +const struct qstr *s2);
> + int (*strncasecmp)(const struct unicode_map *um, const struct qstr *s1,
> +const struct qstr *s2);
> + int (*strncasecmp_folded)(const struct unicode_map *um, const struct
> qstr *cf,
> + const struct qstr *s1);
> + int (*normalize)(const struct unicode_map *um, const struct qstr *str,
> + unsigned char *dest, size_t dlen);
> + int (*casefold)(const struct unicode_map *um, const struct qstr *str,
> + unsigned char *dest, size_t dlen);
> + int (*casefold_hash)(const struct unicode_map *um, const void *salt,
> + struct qstr *str);
> + struct unicode_map* (*load)(const char *version);
> +};
Indirect calls are expensive these days, especially due to the Spectre
mitigations. Would it make sense to use static calls
(include/linux/static_call.h) instead for this?
- Eric
Re: [PATCH v2 3/4] fs: unicode: Use strscpy() instead of strncpy()
On Thu, Mar 18, 2021 at 07:03:04PM +0530, Shreeya Patel wrote:
> Following warning was reported by Kernel Test Robot.
>
> In function 'utf8_parse_version',
> inlined from 'utf8_load' at fs/unicode/utf8mod.c:195:7:
> >> fs/unicode/utf8mod.c:175:2: warning: 'strncpy' specified bound 12 equals
> destination size [-Wstringop-truncation]
> 175 | strncpy(version_string, version, sizeof(version_string));
> | ^~~~
>
> The -Wstringop-truncation warning highlights the unintended
> uses of the strncpy function that truncate the terminating NULL
> character from the source string.
> Unlike strncpy(), strscpy() always null-terminates the destination string,
> hence use strscpy() instead of strncpy().
>
> Signed-off-by: Shreeya Patel
> Reported-by: kernel test robot
> ---
> Changes in v2
> - Resolve warning of -Wstringop-truncation reported by
> kernel test robot.
>
> fs/unicode/unicode-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c
> index d5f09e022ac5..287a8a48836c 100644
> --- a/fs/unicode/unicode-core.c
> +++ b/fs/unicode/unicode-core.c
> @@ -179,7 +179,7 @@ static int unicode_parse_version(const char *version,
> unsigned int *maj,
> {0, NULL}
> };
>
> - strncpy(version_string, version, sizeof(version_string));
> + strscpy(version_string, version, sizeof(version_string));
>
Shouldn't unicode_parse_version() return an error if the string gets truncated
here? I.e. check if strscpy() returns < 0.
Also, this is a "fix" (though one that doesn't currently matter, since 'version'
is currently always shorter than sizeof(version_string)), so it should go first
in the series and have a Fixes tag.
- Eric
Re: [PATCH] fs: proc: fix error return code of proc_map_files_readdir()
On Tue, Mar 09, 2021 at 01:55:27AM -0800, Jia-Ju Bai wrote:
> When get_task_mm() returns NULL to mm, no error return code of
> proc_map_files_readdir() is assigned.
> To fix this bug, ret is assigned with -ENOENT in this case.
>
> Fixes: f0c3b5093add ("[readdir] convert procfs")
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
> ---
> fs/proc/base.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 3851bfcdba56..254cc6ac65fb 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2332,8 +2332,10 @@ proc_map_files_readdir(struct file *file, struct
> dir_context *ctx)
> goto out_put_task;
>
> mm = get_task_mm(task);
> - if (!mm)
> + if (!mm) {
> + ret = -ENOENT;
> goto out_put_task;
> + }
>
> ret = mmap_read_lock_killable(mm);
Is there something in particular that makes you think that returning ENOENT is
the correct behavior in this case? Try 'ls /proc/$pid/map_files' where pid is a
kernel thread; it's an empty directory, which is probably intentional. Your
patch would change reading the directory to fail with ENOENT.
- Eric
Re: [PATCH] crypto: expose needs_key in procfs
On Mon, Mar 01, 2021 at 09:51:56PM +0100, Christoph Böhmwalder wrote:
> > Do you have a specific use case in mind for this information? Normally,
> > users
> > should already know which algorithm they want to use (or set of algorithms
> > they
> > might want to use).
>
> I have a pretty specific use case in mind, yes. For DRBD, we use crypto
> algorithms for peer authentication and for the online-verify mechanism (to
> verify data integrity). The peer authentication algos require a shared
> secret (HMAC), while the verify algorithms are just hash functions without
> keys (we don't configure a shared secret here, so these must explicitly be
> "keyless").
>
> Now, we also have a solution which sits on top of DRBD (LINSTOR), which
> resides purely in userspace. We recently implemented a feature where LINSTOR
> automatically chooses the "best" verify algorithm for all nodes in a
> cluster. It does this by parsing /proc/crypto and prioritizing accordingly.
> The problem is that /proc/crypto currently doesn't contain information about
> whether or not an algorithm requires a key – i.e. whether or not it is
> suitable for DRBD's online-verify mechanism.
>
> See this commit for some context:
> https://github.com/LINBIT/drbd/commit/34ee32e6922994c8e9390859e1790ca
Shouldn't you know ahead of time which algorithm you are using (or set of
algorithms which you might use), and not be parsing /proc/crypto and choosing
some random one (which might be a non-cryptographic algorithm like CRC-32, or
something known to be insecure like MD5)?
Using the algorithm attributes in /proc/crypto only really makes sense if the
decision of which algorithm to use is punted to a higher level and the program
just needs to be able to pass through *any* algorithm available in Linux -- like
how 'cryptsetup' works. But it's preferable to avoid that sort of design, as it
invites users to start depending on weird or insecure things.
> >
> > Also, what about algorithms such as blake2b-256 which optionally take a key
> > (as
> > indicated by CRYPTO_ALG_OPTIONAL_KEY being set)? So it's not really "yes"
> > or
> > "no"; there is a third state as well.
>
> Correct me if I'm missing something, but crypto_shash_alg_needs_key reads:
>
> static inline bool crypto_shash_alg_needs_key(struct shash_alg *alg)
> {
> return crypto_shash_alg_has_setkey(alg) &&
> !(alg->base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY);
> }
>
> So this already accounts for optional keys. It just returns "no" for an
> optional key, which seems like reasonable behavior to me (it doesn't *need*
> a key after all).
>
> Another option would be to make it "yes/no/optional". I'm not sure if that's
> more desirable for most people.
>
BLAKE2 does need a key if it is being used as a keyed hash algorithm. So it
depends on the user, not the algorithm per se.
- Eric
Re: [PATCH] crypto: expose needs_key in procfs
On Mon, Mar 01, 2021 at 05:59:17PM +0100, Christoph Böhmwalder wrote: > Currently, it is not apparent for userspace users which hash algorithms > require a key and which don't. We have /proc/crypto, so add a field > with this information there. > > Signed-off-by: Christoph Böhmwalder > > --- > crypto/shash.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/crypto/shash.c b/crypto/shash.c > index 2e3433ad9762..d3127a0618f2 100644 > --- a/crypto/shash.c > +++ b/crypto/shash.c > @@ -477,6 +477,9 @@ static void crypto_shash_show(struct seq_file *m, struct > crypto_alg *alg) > seq_printf(m, "type : shash\n"); > seq_printf(m, "blocksize: %u\n", alg->cra_blocksize); > seq_printf(m, "digestsize : %u\n", salg->digestsize); > + seq_printf(m, "needs key: %s\n", > +crypto_shash_alg_needs_key(salg) ? > +"yes" : "no"); > } > Do you have a specific use case in mind for this information? Normally, users should already know which algorithm they want to use (or set of algorithms they might want to use). Also, what about algorithms of type "ahash"? Shouldn't this field be added for them too? Also, what about algorithms such as blake2b-256 which optionally take a key (as indicated by CRYPTO_ALG_OPTIONAL_KEY being set)? So it's not really "yes" or "no"; there is a third state as well. - Eric
Re: [PATCH] ext4: Add xattr commands to compat ioctl handler
On Fri, Feb 26, 2021 at 02:14:41PM -0800, Sarthak Kukreti wrote:
> This allows 32-bit userspace utils to use FS_IOC_FSGETXATTR and
> FS_IOC_FSSETXATTR on a 64-bit kernel.
>
> Signed-off-by: Sarthak Kukreti
> ---
> fs/ext4/ioctl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index f0381876a7e5b..055c26296ab46 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1371,6 +1371,8 @@ long ext4_compat_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> return -EFAULT;
> return ext4_ioctl_group_add(file, );
> }
> + case EXT4_IOC_FSGETXATTR:
> + case EXT4_IOC_FSSETXATTR:
> case EXT4_IOC_MOVE_EXT:
> case EXT4_IOC_RESIZE_FS:
> case FITRIM:
These were already added to the list by commit a54d8d34d235
("ext4: Add EXT4_IOC_FSGETXATTR/EXT4_IOC_FSSETXATTR to compat_ioctl").
- Eric
Re: [PATCH] crypto: testmgr - delete some redundant code
On Tue, Feb 23, 2021 at 11:42:04AM +0800, Kai Ye wrote:
> Delete sg_data function, because sg_data function definition same as
> sg_virt(), so need to delete it and use sg_virt() replace to sg_data().
>
> Signed-off-by: Kai Ye
> ---
> crypto/testmgr.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 9335999..e13e73c 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -1168,11 +1168,6 @@ static inline int check_shash_op(const char *op, int
> err,
> return err;
> }
>
> -static inline const void *sg_data(struct scatterlist *sg)
> -{
> - return page_address(sg_page(sg)) + sg->offset;
> -}
> -
> /* Test one hash test vector in one configuration, using the shash API */
> static int test_shash_vec_cfg(const struct hash_testvec *vec,
> const char *vec_name,
> @@ -1230,7 +1225,7 @@ static int test_shash_vec_cfg(const struct hash_testvec
> *vec,
> return 0;
> if (cfg->nosimd)
> crypto_disable_simd_for_test();
> - err = crypto_shash_digest(desc, sg_data(>sgl[0]),
> + err = crypto_shash_digest(desc, sg_virt(>sgl[0]),
> tsgl->sgl[0].length, result);
> if (cfg->nosimd)
> crypto_reenable_simd_for_test();
> @@ -1266,7 +1261,7 @@ static int test_shash_vec_cfg(const struct hash_testvec
> *vec,
> cfg->finalization_type == FINALIZATION_TYPE_FINUP) {
> if (divs[i]->nosimd)
> crypto_disable_simd_for_test();
> - err = crypto_shash_finup(desc, sg_data(>sgl[i]),
> + err = crypto_shash_finup(desc, sg_virt(>sgl[i]),
>tsgl->sgl[i].length, result);
> if (divs[i]->nosimd)
> crypto_reenable_simd_for_test();
> @@ -1278,7 +1273,7 @@ static int test_shash_vec_cfg(const struct hash_testvec
> *vec,
> }
> if (divs[i]->nosimd)
> crypto_disable_simd_for_test();
> - err = crypto_shash_update(desc, sg_data(>sgl[i]),
> + err = crypto_shash_update(desc, sg_virt(>sgl[i]),
> tsgl->sgl[i].length);
> if (divs[i]->nosimd)
> crypto_reenable_simd_for_test();
> --
Looks good,
Reviewed-by: Eric Biggers
- Eric
[GIT PULL] fsverity updates for 5.12
The following changes since commit 6ee1d745b7c9fd573fba142a2efdad76a9f1cb04: Linux 5.11-rc5 (2021-01-24 16:47:14 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git tags/fsverity-for-linus for you to fetch changes up to 07c99001312cbf90a357d4877a358f796eede65b: fs-verity: support reading signature with ioctl (2021-02-07 14:51:19 -0800) Add an ioctl which allows reading fs-verity metadata from a file. This is useful when a file with fs-verity enabled needs to be served somewhere, and the other end wants to do its own fs-verity compatible verification of the file. See the commit messages for details. This new ioctl has been tested using new xfstests I've written for it. Eric Biggers (6): fs-verity: factor out fsverity_get_descriptor() fs-verity: don't pass whole descriptor to fsverity_verify_signature() fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl fs-verity: support reading Merkle tree with ioctl fs-verity: support reading descriptor with ioctl fs-verity: support reading signature with ioctl Documentation/filesystems/fsverity.rst | 76 + fs/ext4/ioctl.c| 7 ++ fs/f2fs/file.c | 11 ++ fs/verity/Makefile | 1 + fs/verity/fsverity_private.h | 13 ++- fs/verity/open.c | 133 ++ fs/verity/read_metadata.c | 195 + fs/verity/signature.c | 20 +--- include/linux/fsverity.h | 12 ++ include/uapi/linux/fsverity.h | 14 +++ 10 files changed, 417 insertions(+), 65 deletions(-) create mode 100644 fs/verity/read_metadata.c
Re: [PATCH v4 5/5] dm: set DM_TARGET_PASSES_CRYPTO feature for some targets
On Mon, Feb 01, 2021 at 05:10:19AM +, Satya Tangirala wrote:
> dm-linear and dm-flakey obviously can pass through inline crypto support.
>
> Co-developed-by: Eric Biggers
> Signed-off-by: Eric Biggers
> Signed-off-by: Satya Tangirala
> ---
> drivers/md/dm-flakey.c | 4 +++-
> drivers/md/dm-linear.c | 5 +++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
> index a2cc9e45cbba..30c6bc151213 100644
> --- a/drivers/md/dm-flakey.c
> +++ b/drivers/md/dm-flakey.c
> @@ -482,8 +482,10 @@ static struct target_type flakey_target = {
> .name = "flakey",
> .version = {1, 5, 0},
> #ifdef CONFIG_BLK_DEV_ZONED
> - .features = DM_TARGET_ZONED_HM,
> + .features = DM_TARGET_ZONED_HM | DM_TARGET_PASSES_CRYPTO,
> .report_zones = flakey_report_zones,
> +#else
> + .features = DM_TARGET_PASSES_CRYPTO,
> #endif
> .module = THIS_MODULE,
> .ctr= flakey_ctr,
> diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
> index 00774b5d7668..fc9c4272c10d 100644
> --- a/drivers/md/dm-linear.c
> +++ b/drivers/md/dm-linear.c
> @@ -229,10 +229,11 @@ static struct target_type linear_target = {
> .version = {1, 4, 0},
> #ifdef CONFIG_BLK_DEV_ZONED
> .features = DM_TARGET_PASSES_INTEGRITY | DM_TARGET_NOWAIT |
> - DM_TARGET_ZONED_HM,
> + DM_TARGET_ZONED_HM | DM_TARGET_PASSES_CRYPTO,
> .report_zones = linear_report_zones,
> #else
> - .features = DM_TARGET_PASSES_INTEGRITY | DM_TARGET_NOWAIT,
> + .features = DM_TARGET_PASSES_INTEGRITY | DM_TARGET_NOWAIT |
> + DM_TARGET_PASSES_CRYPTO,
> #endif
> .module = THIS_MODULE,
> .ctr= linear_ctr,
> --
This latest version looks good to me. If it's needed despite my
Co-developed-by, feel free to add:
Reviewed-by: Eric Biggers
BTW, the #ifdef for CONFIG_BLK_DEV_ZONED is error-prone (though it's a
pre-existing issue). I wonder if DM_TARGET_ZONED_HM should be defined to 0 when
!CONFIG_BLK_DEV_ZONED, which would avoid the need for the #ifdef on .features.
- Eric
Re: [PATCH v4 4/5] dm: support key eviction from keyslot managers of underlying devices
On Mon, Feb 01, 2021 at 05:10:18AM +, Satya Tangirala wrote:
> Now that device mapper supports inline encryption, add the ability to
> evict keys from all underlying devices. When an upper layer requests
> a key eviction, we simply iterate through all underlying devices
> and evict that key from each device.
>
> Co-developed-by: Eric Biggers
> Signed-off-by: Eric Biggers
> Signed-off-by: Satya Tangirala
This latest version looks good to me. If it's needed despite my
Co-developed-by, feel free to add:
Reviewed-by: Eric Biggers
- Eric
Re: [PATCH v4 3/5] dm: add support for passing through inline crypto support
On Mon, Feb 01, 2021 at 05:10:17AM +, Satya Tangirala wrote:
> Update the device-mapper core to support exposing the inline crypto
> support of the underlying device(s) through the device-mapper device.
>
> This works by creating a "passthrough keyslot manager" for the dm
> device, which declares support for encryption settings which all
> underlying devices support. When a supported setting is used, the bio
> cloning code handles cloning the crypto context to the bios for all the
> underlying devices. When an unsupported setting is used, the blk-crypto
> fallback is used as usual.
>
> Crypto support on each underlying device is ignored unless the
> corresponding dm target opts into exposing it. This is needed because
> for inline crypto to semantically operate on the original bio, the data
> must not be transformed by the dm target. Thus, targets like dm-linear
> can expose crypto support of the underlying device, but targets like
> dm-crypt can't. (dm-crypt could use inline crypto itself, though.)
>
> A DM device's table can only be changed if the "new" inline encryption
> capabilities are a (*not* necessarily strict) superset of the "old" inline
> encryption capabilities. Attempts to make changes to the table that result
> in some inline encryption capability becoming no longer supported will be
> rejected.
>
> For the sake of clarity, key eviction from underlying devices will be
> handled in a future patch.
>
> Co-developed-by: Eric Biggers
> Signed-off-by: Eric Biggers
> Signed-off-by: Satya Tangirala
I don't see any obvious issues with this latest version. I assume you've tested
it on real hardware?
If it's needed despite my Co-developed-by, feel free to add:
Reviewed-by: Eric Biggers
A few nits about comments, in case you resend:
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 086d293c2b03..bf3e66f39a4a 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -162,6 +163,10 @@ struct dm_table {
> void *event_context;
>
> struct dm_md_mempools *mempools;
> +
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> + struct blk_keyslot_manager *ksm;
> +#endif
> };
It might be helpful if there was a brief comment here that explained that this
field is only set temporarily while the table is being set up, and it gets set
to NULL after the capabilities have been transferred to the request_queue.
I.e., it's not something that stays around here while the dm device is active.
> +/*
> + * Constructs and returns a keyslot manager that represents the crypto
> + * capabilities of the devices described by the dm_table. However, if the
> + * constructed keyslot manager does not support a superset of the crypto
> + * capabilities supported by the current keyslot manager of the
> mapped_device,
> + * it returns an error instead, since we don't support restricting crypto
> + * capabilities on table changes. Finally, if the constructed keyslot manager
> + * doesn't actually support any crypto modes at all, it just returns NULL.
> + */
> +static int
> +dm_table_construct_keyslot_manager(struct dm_table *t)
This doesn't "return" the keyslot manager anymore, but rather assigns it to
t->ksm. It would also be helpful if the comment explicitly mentioned that the
goal is to find the capabilities that all the devices have in common.
E.g. "Initializes t->ksm with a keyslot manager that represents the common set
of crypto capabilities of the devices described by the dm_table.".
> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> index 61a66fb8ebb3..d2142f5a82a7 100644
> --- a/include/linux/device-mapper.h
> +++ b/include/linux/device-mapper.h
> @@ -257,6 +257,12 @@ struct target_type {
> #define DM_TARGET_NOWAIT 0x0080
> #define dm_target_supports_nowait(type) ((type)->features & DM_TARGET_NOWAIT)
>
> +/*
> + *
> + */
> +#define DM_TARGET_PASSES_CRYPTO 0x0100
> +#define dm_target_passes_crypto(type) ((type)->features &
> DM_TARGET_PASSES_CRYPTO)
The above comment isn't very useful :-)
- Eric
Re: [PATCH v4 2/5] block: keyslot-manager: Introduce functions for device mapper support
On Mon, Feb 01, 2021 at 05:10:16AM +, Satya Tangirala wrote: > Introduce blk_ksm_update_capabilities() to update the capabilities of > a keyslot manager (ksm) in-place. The pointer to a ksm in a device's > request queue may not be easily replaced, because upper layers like > the filesystem might access it (e.g. for programming keys/checking > capabilities) at the same time the device wants to replace that > request queue's ksm (and free the old ksm's memory). This function > allows the device to update the capabilities of the ksm in its request > queue directly. Devices can safely update the ksm this way without any > synchronization with upper layers *only* if the updated (new) ksm > continues to support all the crypto capabilities that the old ksm did > (see description below for blk_ksm_is_superset() for why this is so). > > Also introduce blk_ksm_is_superset() which checks whether one ksm's > capabilities are a (not necessarily strict) superset of another ksm's. > The blk-crypto framework requires that crypto capabilities that were > advertised when a bio was created continue to be supported by the > device until that bio is ended - in practice this probably means that > a device's advertised crypto capabilities can *never* "shrink" (since > there's no synchronization between bio creation and when a device may > want to change its advertised capabilities) - so a previously > advertised crypto capability must always continue to be supported. > This function can be used to check that a new ksm is a valid > replacement for an old ksm. > > Signed-off-by: Satya Tangirala Looks good, you can add: Reviewed-by: Eric Biggers
Re: [PATCH v5 05/11] crypto: qce: skcipher: Return error for zero length messages
On Thu, Feb 04, 2021 at 07:09:53PM -0500, Thara Gopinath wrote: > > > @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct > > > skcipher_request *req, int encrypt) > > > rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; > > > keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : > > > ctx->enc_keylen; > > > + /* CE does not handle 0 length messages */ > > > + if (!req->cryptlen) > > > + return -EOPNOTSUPP; > > > + > > > > For the algorithms in question, the correct behavior is to return 0. > > What do you mean? The driver should return a 0 ? Yes, there is nothing to do for empty inputs, so just return 0 (success). > > Aren't the tests catching that difference? > > I was anyways planning on sending an email to the list with these queries. > But since you asked, these are my observations with fuzz testing which I > have been doing quite a bit now (I am also working on adding a few qualcomm > AEAD algorithms support in mainline). > > - if the generic algorithm supports 0 length messages and the transformation > I am testing does not, the test framework throws an error and stops. > - key support mismatch between the generic algorithm vs my algorithm /engine > also does the same thing.For eg, Qualcomm CE engine does not support any > three keys being same for triple des algorithms. Where as a two key 3des is > a valid scenario for generic algorithm(k1=k3). Another example is hardware > engine not supporting AES192. > > How are these scenarios usually handled ? Why not allow the test framework > to proceed with the testing if the algorithm does not support a particular > scenario ? Omitting support for certain inputs isn't allowed. Anyone in the kernel who wants to use a particular algorithm could get this driver for it, and if they happen to use inputs which the driver decided not to support, things will break. The way that drivers handle this is to use a fallback cipher for inputs they don't support. - Eric
Re: [PATCH v5 06/11] crypto: qce: skcipher: Return error for non-blocksize data(ECB/CBC algorithms)
On Thu, Feb 04, 2021 at 04:43:54PM -0500, Thara Gopinath wrote: > + /* > + * ECB and CBC algorithms require message lengths to be > + * multiples of block size. > + * TODO: The spec says AES CBC mode for certain versions > + * of crypto engine can handle partial blocks as well. > + * Test and enable such messages. > + */ > + if (IS_ECB(rctx->flags) || IS_CBC(rctx->flags)) > + if (!IS_ALIGNED(req->cryptlen, blocksize)) > + return -EINVAL; CBC by definition only operates on full blocks, so the TODO doesn't make sense. Is the partial block support really CTS-CBC? - Eric
Re: [PATCH v5 05/11] crypto: qce: skcipher: Return error for zero length messages
On Thu, Feb 04, 2021 at 04:43:53PM -0500, Thara Gopinath wrote: > Crypto engine BAM dma does not support 0 length data. Return unsupported > if zero length messages are passed for transformation. > > Signed-off-by: Thara Gopinath > --- > drivers/crypto/qce/skcipher.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c > index de1f37ed4ee6..331b3c3a5b59 100644 > --- a/drivers/crypto/qce/skcipher.c > +++ b/drivers/crypto/qce/skcipher.c > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request > *req, int encrypt) > rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; > keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; > > + /* CE does not handle 0 length messages */ > + if (!req->cryptlen) > + return -EOPNOTSUPP; > + For the algorithms in question, the correct behavior is to return 0. Aren't the tests catching that difference? - Eric
Re: [PATCH RESEND] random: fix the RNDRESEEDCRNG ioctl
On Tue, Jan 12, 2021 at 11:28:18AM -0800, Eric Biggers wrote:
> From: Eric Biggers
>
> The RNDRESEEDCRNG ioctl reseeds the primary_crng from itself, which
> doesn't make sense. Reseed it from the input_pool instead.
>
> Fixes: d848e5f8e1eb ("random: add new ioctl RNDRESEEDCRNG")
> Cc: sta...@vger.kernel.org
> Cc: linux-cry...@vger.kernel.org
> Cc: Andy Lutomirski
> Cc: Jann Horn
> Cc: Theodore Ts'o
> Reviewed-by: Jann Horn
> Signed-off-by: Eric Biggers
> ---
>
> Andrew, please consider taking this patch since the maintainer has been
> ignoring it for 4 months
> (https://lkml.kernel.org/lkml/20200916041908.66649-1-ebigg...@kernel.org/T/#u).
Ping.
Re: [PATCH RESEND] random: initialize ChaCha20 constants with correct endianness
On Tue, Jan 12, 2021 at 11:29:27AM -0800, Eric Biggers wrote: > From: Eric Biggers > > On big endian CPUs, the ChaCha20-based CRNG is using the wrong > endianness for the ChaCha20 constants. > > This doesn't matter cryptographically, but technically it means it's not > ChaCha20 anymore. Fix it to always use the standard constants. > > Cc: linux-cry...@vger.kernel.org > Cc: Andy Lutomirski > Cc: Jann Horn > Cc: Theodore Ts'o > Acked-by: Herbert Xu > Signed-off-by: Eric Biggers > --- > > Andrew, please consider taking this patch since the maintainer has been > ignoring it for 4 months > (https://lkml.kernel.org/lkml/20200916045013.142179-1-ebigg...@kernel.org/T/#u). Ping.
Re: [PATCH RESEND] random: remove dead code left over from blocking pool
On Tue, Jan 12, 2021 at 11:29:38AM -0800, Eric Biggers wrote:
> From: Eric Biggers
>
> Remove some dead code that was left over following commit 90ea1c6436d2
> ("random: remove the blocking pool").
>
> Cc: linux-cry...@vger.kernel.org
> Cc: Andy Lutomirski
> Cc: Jann Horn
> Cc: Theodore Ts'o
> Reviewed-by: Andy Lutomirski
> Signed-off-by: Eric Biggers
> ---
>
> Andrew, please consider taking this patch since the maintainer has been
> ignoring it for 4 months
> (https://lkml.kernel.org/lkml/20200916043652.96640-1-ebigg...@kernel.org/T/#u).
>
Ping.
Re: [PATCH v2 3/3] f2fs: Add metadata encryption support
On Thu, Dec 17, 2020 at 03:04:35PM +, Satya Tangirala wrote:
> Wire up metadata encryption support with the fscrypt metadata crypt
> additions. Note that this feature relies on the blk-crypto framework
> for encryption, and thus requires either hardware inline encryption
> support or the blk-crypto-fallback.
>
> Filesystems can be configured with metadata encryption support using the
> f2fs userspace tools (mkfs.f2fs). Once formatted, F2FS filesystems with
> metadata encryption can be mounted as long as the required key is present.
> fscrypt looks for a logon key with the key descriptor=
> fscrypt:. The metadata_key_identifier is stored in
> the filesystem superblock (and the userspace tools print the required
> key descriptor).
>
> Right now, the superblock of the filesystem is itself not encrypted. F2FS
> reads the superblock using sb_bread, which uses the bd_inode of the block
> device as the address space for any data it reads from the block device -
> the data read under the bd_inode address space must be what is physically
> present on disk (i.e. if the superblock is encrypted, then the ciphertext
> of the superblock must be present in the page cache in the bd_inode's
> address space), but f2fs requires that the superblock is decrypted by
> blk-crypto, which would put the decrypted page contents into the page cache
> instead. We could make f2fs read the superblock by submitting bios directly
> with a separate address space, but we choose to just not encrypt the
> superblock for now.
>
> Not encrypting the superblock allows us to store the encryption algorithm
> used for metadata encryption within the superblock itself, which simplifies
> a few things. The userspace tools will store the encryption algorithm in
> the superblock when formatting the FS.
The explanation about why the superblock isn't encrypted seems a bit backwards.
It makes it sound like this decision was mainly an accident because of how f2fs
is currently implemented. But actually we need to leave the superblock
unencrypted anyway in order to keep the filesystem type and metadata encryption
options readable from disk, so that the filesystem can be mounted without
knowing the filesystem type and encryption options ahead of time -- right?
I.e. would anything actually be different if it was super easy to encrypt the
superblock in the kernel?
>
> + /* Check if FS has metadata encryption if kernel doesn't support it */
> +#ifndef CONFIG_FS_ENCRYPTION_METADATA
> + if (raw_super->metadata_crypt_alg) {
> + f2fs_err(sbi, "Filesystem has metadata encryption but kernel
> support for it wasn't enabled");
> + return -EINVAL;
> + }
> +#endif
This can use !IS_ENABLED(CONFIG_FS_ENCRYPTION_METADATA).
> + if (fscrypt_metadata_crypted(sb)) {
> + f2fs_notice(sbi, "Mounted with metadata key identifier =
> %s%*phN",
> + FSCRYPT_KEY_DESC_PREFIX,
> + FSCRYPT_KEY_IDENTIFIER_SIZE,
> + sbi->raw_super->metadata_crypt_key_ident);
> + }
Should this show the encryption algorithm too? Maybe:
"Metadata encryption enabled; algorithm=%s, key_identifier=%*phN"
Note that showing the "fscrypt:" key description prefix doesn't really add
anything, so I recommend leaving it out.
> + /* The metadata encryption algorithm (FSCRYPT_MODE_*) */
... or 0 if none.
> + __le32 metadata_crypt_alg;
> + /* The metadata encryption key identifier */
> + __u8 metadata_crypt_key_ident[FSCRYPT_KEY_IDENTIFIER_SIZE];
... (if metadata_crypt_alg != 0)
- Eric
Re: [PATCH v2 2/3] fscrypt: Add metadata encryption support
On Thu, Dec 17, 2020 at 03:04:34PM +, Satya Tangirala wrote: > Introduces functions that help with metadata encryption. > > In particular, we introduce: > > fscrypt_setup_metadata_encryption() - filesystems should call this function > to set up metadata encryption on a super block with the encryption > algorithm (the desired FSCRYPT_MODE_*) and the key identifier of the > encryption key.fscrypt looks for a logon key with the specified key > identifier with prefix "fscrypt:". fscrypt will verify that the key > identifier matches the identifier that is derived using HKDF-512 with the > logon key as the keying material, no salt, and > "fscrypt\0|HKDF_CONTEXT_KEY_IDENTIFIER" as the info string. This describes *what* is done, but not *why*. The why is that we want to ensure that wrong keys get rejected early on, before the filesystem is mounted. Also, what happens if we want to add support for a KDF other than HKDF-SHA512 in the future? With the existing fscrypt use of HKDF-SHA512, we can add support for more KDFs by using one of the reserved fields in fscrypt_policy_v2 and fscrypt_add_key_arg to indicate the KDF version. But as proposed, metadata encryption has no reserved fields in the filesystem superblock (that are strictly validated). It might be a good idea to add some reserved fields. > Filesystems should call fscrypt_set_bio_crypt_ctx() on any bio that needs > either metadata or file contents encryption. fscrypt will choose the > appropriate key (based on the inode argument) to use for encrypting the > bio. Filesystem metadata doesn't necessarily have an inode associated with it. How does that work? > diff --git a/Documentation/filesystems/fscrypt.rst > b/Documentation/filesystems/fscrypt.rst > index 44b67ebd6e40..708164c413cc 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -27,8 +27,6 @@ at the block device level. This allows it to encrypt > different files > with different keys and to have unencrypted files on the same > filesystem. This is useful for multi-user systems where each user's > data-at-rest needs to be cryptographically isolated from the others. > -However, except for filenames, fscrypt does not encrypt filesystem > -metadata. > > Unlike eCryptfs, which is a stacked filesystem, fscrypt is integrated > directly into supported filesystems --- currently ext4, F2FS, and > @@ -47,6 +45,15 @@ userspace provides the key, all regular files, > directories, and > symbolic links created in that directory tree are transparently > encrypted. > > +fscrypt also has support for encrypting filesystem metadata, which > +can be used independently of file contents encryption. For any > +filesystem block that isn't part of an encrypted file's contents, > +the filesystem can ask fscrypt to encrypt it with the metadata encryption > +key set up ahead of time. In general, filesystems should always choose > +to encrypt a filesystem block with the metadata encryption key, if > +that block isn't already encrypted with another key (filesystems may > +choose to leave certain blocks, like the superblock, unencrypted). We need to be very careful how we describe the metadata encryption feature because it's not really self-explanatory; people are going to wonder: - What is meant by "metadata"? - How does it relate to dm-crypt, and why not just use dm-crypt? - How does it interact with the existing filenames encryption? - How does it interact with unencrypted files? We need to explain it properly so that all these questions are answered. I recommend adding metadata encryption as a top-level section in the documentation file (not just in the Introduction and Implementation Details sections as this patch currently proposes) and adding a proper explanation of it from a user's perspective. The Introduction should be kept brief. > +fscrypt protects the confidentiality of non-filename metadata, e.g. > +file sizes, file permissions, file timestamps, and extended attribute > +only when metadata encryption support is enabled for the filesystem, > +and the filesystem chooses to protect such information with the > +metadata encryption key. Do we actually anticipate that filesystems might support metadata encryption but not encrypt these types of metadata? That would be weird, since the purpose of metadata encryption is basically to ensure that everything gets encrypted. > +For v2 encryption policies, if the filesystem has a metadata crypt key, > +the master key is first "mixed" with the metadata crypt key, generating > +a "mixed-metadata key", which is then used in place of the master key > +in the process described above. The "mixed-metadata key" is generated > +by using the metadata crypt key as the input keying material, and > +a context specific byte and the original master key as the > +application-specific information string with HKDF-SHA512 (refer to > +fscrypt_mix_in_metadata_key() for details). This explains *what* is done, but
Re: [PATCH v2 7/7] fs: HKDF - remove duplicate memory clearing
On Sun, Jan 24, 2021 at 03:04:50PM +0100, Stephan Müller wrote: > The clearing of the OKM memory buffer in case of an error is already > performed by the HKDF implementation crypto_hkdf_expand. Thus, the > code clearing is not needed any more in the file system code base. > > Signed-off-by: Stephan Mueller > --- > fs/crypto/hkdf.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c > index ae236b42b1f0..c48dd8ca3a46 100644 > --- a/fs/crypto/hkdf.c > +++ b/fs/crypto/hkdf.c > @@ -102,13 +102,10 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf > *hkdf, u8 context, > .iov_base = (u8 *)info, > .iov_len = infolen, > } }; > - int err = crypto_hkdf_expand(hkdf->hmac_tfm, > - info_iov, ARRAY_SIZE(info_iov), > - okm, okmlen); > > - if (unlikely(err)) > - memzero_explicit(okm, okmlen); /* so caller doesn't need to */ > - return err; > + return crypto_hkdf_expand(hkdf->hmac_tfm, > + info_iov, ARRAY_SIZE(info_iov), > + okm, okmlen); > } > Shoudn't this just be folded into the previous patch, which converted fscrypt_hkdf_expand() to use crypto_hkdf_expand() in the first place? - Eric
Re: [PATCH v2 6/7] fs: use HKDF implementation from kernel crypto API
On Sun, Jan 24, 2021 at 03:04:31PM +0100, Stephan Müller wrote:
> @@ -74,16 +57,14 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8
> *master_key,
> return PTR_ERR(hmac_tfm);
> }
>
> - if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != sizeof(prk))) {
> + if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != HKDF_HASHLEN)) {
> err = -EINVAL;
> goto err_free_tfm;
> }
>
> - err = hkdf_extract(hmac_tfm, master_key, master_key_size, prk);
> - if (err)
> - goto err_free_tfm;
> -
> - err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk));
> + /* HKDF-Extract (RFC 5869 section 2.2), unsalted */
> + err = crypto_hkdf_extract(hmac_tfm, NULL, 0,
> + master_key, master_key_size);
> if (err)
> goto err_free_tfm;
>
> @@ -93,7 +74,6 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8
> *master_key,
> err_free_tfm:
> crypto_free_shash(hmac_tfm);
> out:
> - memzero_explicit(prk, sizeof(prk));
> return err;
> }
The 'out' label isn't needed anymore. 'goto out' should be replaced with
'return 0'.
- Eric
Re: [PATCH v2 6/7] fs: use HKDF implementation from kernel crypto API
Please prefix the commit subject with "fscrypt: " rather than "fs: ".
On Sun, Jan 24, 2021 at 03:04:31PM +0100, Stephan Müller wrote:
> diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
> index e0ec21055505..ae236b42b1f0 100644
> --- a/fs/crypto/hkdf.c
> +++ b/fs/crypto/hkdf.c
> @@ -9,7 +9,7 @@
> * Copyright 2019 Google LLC
> */
>
> -#include
> +#include
> #include
>
> #include "fscrypt_private.h"
> @@ -37,23 +37,7 @@
> * unnecessarily long master keys. Thus fscrypt still does HKDF-Extract. No
> * salt is used, since fscrypt master keys should already be pseudorandom and
> * there's no way to persist a random salt per master key from kernel mode.
> - */
> -
> -/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
> -static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
> - unsigned int ikmlen, u8 prk[HKDF_HASHLEN])
> -{
> - static const u8 default_salt[HKDF_HASHLEN];
> - int err;
> -
> - err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN);
> - if (err)
> - return err;
> -
> - return crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
> -}
> -
> -/*
> + *
> * Compute HKDF-Extract using the given master key as the input keying
> material,
> * and prepare an HMAC transform object keyed by the resulting pseudorandom
> key.
> *
I don't think this comment should be joined with the one above it. The earlier
comment describes the general approach taken with fscrypt and HKDF (including
all steps), while the one beginning with "Compute HKDF-Extract" describes
fscrypt_init_hkdf() specifically.
- Eric
Re: [PATCH v2 3/7] crypto: add RFC5869 HKDF
On Sun, Jan 24, 2021 at 03:03:28PM +0100, Stephan Müller wrote:
> RFC5869 specifies an extract and expand two-step key derivation
> function. The HKDF implementation is provided as a service function that
> operates on a caller-provided HMAC handle. The caller has to allocate
> the HMAC shash handle and then can invoke the HKDF service functions.
> The HKDF implementation ensures that the entire state is kept with the
> HMAC shash handle which implies that no additional state is required to
> be maintained by the HKDF implementation.
>
> The extract function is invoked via the crypto_hkdf_extract call. RFC5869
> allows two optional parameters to be provided to the extract operation:
> the salt and input key material (IKM). Both are to be provided with the
> seed parameter where the salt is the first entry of the seed parameter
> and all subsequent entries are handled as IKM. If the caller intends to
> invoke the HKDF without salt, it has to provide a NULL/0 entry as first
> entry in seed.
The part about the "seed parameter" is outdated.
> The expand function is invoked via crypto_hkdf_expand and can be
> invoked multiple times. This function allows the caller to provide a
> context for the key derivation operation. As specified in RFC5869, it is
> optional. In case such context is not provided, the caller must provide
> NULL / 0 for the info / info_nvec parameters.
This commit message doesn't actually mention of *why* this patch is useful. Is
it because there are going to be more uses of HKDF in the kernel besides the one
in fs/crypto/? If so, what are those planned uses?
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 9f375c2350f5..661287d7283b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1862,6 +1862,13 @@ config CRYPTO_JITTERENTROPY
> random numbers. This Jitterentropy RNG registers with
> the kernel crypto API and can be used by any caller.
>
> +config CRYPTO_HKDF
> + tristate "Extract and Expand HKDF (RFC 5869)"
> + select CRYPTO_HASH
> + help
> + Enable the extract and expand key derivation function compliant
> + to RFC 5869.
This is just a library function, so it shouldn't be user-selectable.
It should just be selected by the kconfig options that need it.
> diff --git a/crypto/hkdf.c b/crypto/hkdf.c
> new file mode 100644
> index ..8e80eca202e7
> --- /dev/null
> +++ b/crypto/hkdf.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * HMAC-based Extract-and-Expand Key Derivation Function (conformant to
> RFC5869)
> + *
> + * Copyright (C) 2020, Stephan Mueller
> + */
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * HKDF expand phase
> + */
> +int crypto_hkdf_expand(struct crypto_shash *kmd,
> +const struct kvec *info, unsigned int info_nvec,
> +u8 *dst, unsigned int dlen)
> +{
> + SHASH_DESC_ON_STACK(desc, kmd);
> + const unsigned int h = crypto_shash_digestsize(kmd), dlen_orig = dlen;
> + unsigned int i;
> + int err = 0;
> + u8 *dst_orig = dst;
> + const u8 *prev = NULL;
> + u8 ctr = 0x01;
> +
> + if (dlen > h * 255)
> + return -EINVAL;
> +
> + desc->tfm = kmd;
> +
> + /* T(1) and following */
> + while (dlen) {
> + err = crypto_shash_init(desc);
> + if (err)
> + goto out;
> +
> + if (prev) {
> + err = crypto_shash_update(desc, prev, h);
> + if (err)
> + goto out;
> + }
> +
> + for (i = 0; i < info_nvec; i++) {
> + err = crypto_shash_update(desc, info[i].iov_base,
> + info[i].iov_len);
> + if (err)
> + goto out;
> + }
> +
> + if (dlen < h) {
> + u8 tmpbuffer[HASH_MAX_DIGESTSIZE];
> +
> + err = crypto_shash_finup(desc, , 1, tmpbuffer);
> + if (err)
> + goto out;
> + memcpy(dst, tmpbuffer, dlen);
> + memzero_explicit(tmpbuffer, h);
> + goto out;
> + }
> +
> + err = crypto_shash_finup(desc, , 1, dst);
> + if (err)
> + goto out;
> +
> + prev = dst;
> + dst += h;
> + dlen -= h;
> + ctr++;
> + }
> +
> +out:
> + if (err)
> + memzero_explicit(dst_orig, dlen_orig);
> + shash_desc_zero(desc);
> + return err;
> +}
> +EXPORT_SYMBOL(crypto_hkdf_expand);
EXPORT_SYMBOL_GPL?
> +
> +/*
> + * HKDF extract phase.
> + */
> +int crypto_hkdf_extract(struct crypto_shash *kmd,
> + const u8 *salt, size_t saltlen,
> + const u8 *ikm, size_t ikmlen)
> +{
> + SHASH_DESC_ON_STACK(desc, kmd);
> + unsigned int h =
Re: [dm-devel] [PATCH AUTOSEL 5.4 03/26] dm integrity: select CRYPTO_SKCIPHER
On Tue, Jan 19, 2021 at 08:26:40PM -0500, Sasha Levin wrote: > From: Anthony Iliopoulos > > [ Upstream commit f7b347acb5f6c29d9229bb64893d8b6a2c7949fb ] > > The integrity target relies on skcipher for encryption/decryption, but > certain kernel configurations may not enable CRYPTO_SKCIPHER, leading to > compilation errors due to unresolved symbols. Explicitly select > CRYPTO_SKCIPHER for DM_INTEGRITY, since it is unconditionally dependent > on it. > > Signed-off-by: Anthony Iliopoulos > Signed-off-by: Mike Snitzer > Signed-off-by: Sasha Levin > --- > drivers/md/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig > index aa98953f4462e..7dd6e98257c72 100644 > --- a/drivers/md/Kconfig > +++ b/drivers/md/Kconfig > @@ -565,6 +565,7 @@ config DM_INTEGRITY > select BLK_DEV_INTEGRITY > select DM_BUFIO > select CRYPTO > + select CRYPTO_SKCIPHER > select ASYNC_XOR > ---help--- > This device-mapper target emulates a block device that has CRYPTO_SKCIPHER doesn't exist in 5.4 and earlier because it was renamed from CRYPTO_BLKCIPHER in 5.5. If this patch is really important enough to backport, CRYPTO_SKCIPHER will need to be changed to CRYPTO_BLKCIPHER. - Eric
Re: [RFC V1 3/7] crypto: ghash - Optimized GHASH computations
On Fri, Jan 15, 2021 at 04:20:44PM -0800, Dave Hansen wrote: > On 1/15/21 4:14 PM, Dey, Megha wrote: > > Also, I do not know of any cores that implement PCLMULQDQ and not AES-NI. > > That's true, bit it's also possible that a hypervisor could enumerate > support for PCLMULQDQ and not AES-NI. In general, we've tried to > implement x86 CPU features independently, even if they never show up in > a real CPU independently. We only add optimized implementations of crypto algorithms if they are actually useful, though. If they would never be used in practice, that's not useful. - Eric
Re: [RFC V1 3/7] crypto: ghash - Optimized GHASH computations
On Fri, Jan 15, 2021 at 04:14:40PM -0800, Dey, Megha wrote: > > Hello Megha, > > > > What is the purpose of this separate GHASH module? GHASH is only used > > in combination with AES-CTR to produce GCM, and this series already > > contains a GCM driver. > > > > Do cores exist that implement PCLMULQDQ but not AES-NI? > > > > If not, I think we should be able to drop this patch (and remove the > > existing PCLMULQDQ GHASH driver as well) > > AFAIK, dm-verity (authenticated but not encrypted file system) is one use > case for authentication only. > > Although I am not sure if GHASH is specifically used for this or SHA? > > Also, I do not know of any cores that implement PCLMULQDQ and not AES-NI. > dm-verity only uses unkeyed hash algorithms. So no, it doesn't use GHASH. - Eric
[PATCH RESEND] random: fix the RNDRESEEDCRNG ioctl
From: Eric Biggers
The RNDRESEEDCRNG ioctl reseeds the primary_crng from itself, which
doesn't make sense. Reseed it from the input_pool instead.
Fixes: d848e5f8e1eb ("random: add new ioctl RNDRESEEDCRNG")
Cc: sta...@vger.kernel.org
Cc: linux-cry...@vger.kernel.org
Cc: Andy Lutomirski
Cc: Jann Horn
Cc: Theodore Ts'o
Reviewed-by: Jann Horn
Signed-off-by: Eric Biggers
---
Andrew, please consider taking this patch since the maintainer has been
ignoring it for 4 months
(https://lkml.kernel.org/lkml/20200916041908.66649-1-ebigg...@kernel.org/T/#u).
drivers/char/random.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 5f3b8ac9d97b0..a894c0559a8cf 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1972,7 +1972,7 @@ static long random_ioctl(struct file *f, unsigned int
cmd, unsigned long arg)
return -EPERM;
if (crng_init < 2)
return -ENODATA;
- crng_reseed(_crng, NULL);
+ crng_reseed(_crng, _pool);
crng_global_init_time = jiffies - 1;
return 0;
default:
--
2.30.0
[PATCH RESEND] random: remove dead code left over from blocking pool
From: Eric Biggers
Remove some dead code that was left over following commit 90ea1c6436d2
("random: remove the blocking pool").
Cc: linux-cry...@vger.kernel.org
Cc: Andy Lutomirski
Cc: Jann Horn
Cc: Theodore Ts'o
Reviewed-by: Andy Lutomirski
Signed-off-by: Eric Biggers
---
Andrew, please consider taking this patch since the maintainer has been
ignoring it for 4 months
(https://lkml.kernel.org/lkml/20200916043652.96640-1-ebigg...@kernel.org/T/#u).
drivers/char/random.c | 17 ++-
include/trace/events/random.h | 83 ---
2 files changed, 3 insertions(+), 97 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index a894c0559a8cf..bbc5098b1a81f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -500,7 +500,6 @@ struct entropy_store {
unsigned short add_ptr;
unsigned short input_rotate;
int entropy_count;
- unsigned int initialized:1;
unsigned int last_data_init:1;
__u8 last_data[EXTRACT_SIZE];
};
@@ -660,7 +659,7 @@ static void process_random_ready_list(void)
*/
static void credit_entropy_bits(struct entropy_store *r, int nbits)
{
- int entropy_count, orig, has_initialized = 0;
+ int entropy_count, orig;
const int pool_size = r->poolinfo->poolfracbits;
int nfrac = nbits << ENTROPY_SHIFT;
@@ -717,23 +716,14 @@ static void credit_entropy_bits(struct entropy_store *r,
int nbits)
if (cmpxchg(>entropy_count, orig, entropy_count) != orig)
goto retry;
- if (has_initialized) {
- r->initialized = 1;
- kill_fasync(, SIGIO, POLL_IN);
- }
-
trace_credit_entropy_bits(r->name, nbits,
entropy_count >> ENTROPY_SHIFT, _RET_IP_);
if (r == _pool) {
int entropy_bits = entropy_count >> ENTROPY_SHIFT;
- if (crng_init < 2) {
- if (entropy_bits < 128)
- return;
+ if (crng_init < 2 && entropy_bits >= 128)
crng_reseed(_crng, r);
- entropy_bits = ENTROPY_BITS(r);
- }
}
}
@@ -1385,8 +1375,7 @@ static size_t account(struct entropy_store *r, size_t
nbytes, int min,
}
/*
- * This function does the actual extraction for extract_entropy and
- * extract_entropy_user.
+ * This function does the actual extraction for extract_entropy.
*
* Note: we assume that .poolwords is a multiple of 16 words.
*/
diff --git a/include/trace/events/random.h b/include/trace/events/random.h
index 9570a10cb949b..3d7b432ca5f31 100644
--- a/include/trace/events/random.h
+++ b/include/trace/events/random.h
@@ -85,28 +85,6 @@ TRACE_EVENT(credit_entropy_bits,
__entry->entropy_count, (void *)__entry->IP)
);
-TRACE_EVENT(push_to_pool,
- TP_PROTO(const char *pool_name, int pool_bits, int input_bits),
-
- TP_ARGS(pool_name, pool_bits, input_bits),
-
- TP_STRUCT__entry(
- __field( const char *, pool_name )
- __field( int, pool_bits )
- __field( int, input_bits )
- ),
-
- TP_fast_assign(
- __entry->pool_name = pool_name;
- __entry->pool_bits = pool_bits;
- __entry->input_bits = input_bits;
- ),
-
- TP_printk("%s: pool_bits %d input_pool_bits %d",
- __entry->pool_name, __entry->pool_bits,
- __entry->input_bits)
-);
-
TRACE_EVENT(debit_entropy,
TP_PROTO(const char *pool_name, int debit_bits),
@@ -161,35 +139,6 @@ TRACE_EVENT(add_disk_randomness,
MINOR(__entry->dev), __entry->input_bits)
);
-TRACE_EVENT(xfer_secondary_pool,
- TP_PROTO(const char *pool_name, int xfer_bits, int request_bits,
-int pool_entropy, int input_entropy),
-
- TP_ARGS(pool_name, xfer_bits, request_bits, pool_entropy,
- input_entropy),
-
- TP_STRUCT__entry(
- __field( const char *, pool_name )
- __field( int, xfer_bits )
- __field( int, request_bits)
- __field( int, pool_entropy)
- __field( int, input_entropy )
- ),
-
- TP_fast_assign(
- __entry->pool_name = pool_name;
- __entry->xfer_bits = xfer_bits;
- __entry->request_bits = request_bits;
- __entry->pool_entropy = pool_entropy;
- __entry->input_entropy = input_entropy;
- ),
-
- TP_printk("pool %s xfer_bits %d request_bits %d pool_entropy %d "
-
[PATCH RESEND] random: initialize ChaCha20 constants with correct endianness
From: Eric Biggers
On big endian CPUs, the ChaCha20-based CRNG is using the wrong
endianness for the ChaCha20 constants.
This doesn't matter cryptographically, but technically it means it's not
ChaCha20 anymore. Fix it to always use the standard constants.
Cc: linux-cry...@vger.kernel.org
Cc: Andy Lutomirski
Cc: Jann Horn
Cc: Theodore Ts'o
Acked-by: Herbert Xu
Signed-off-by: Eric Biggers
---
Andrew, please consider taking this patch since the maintainer has been
ignoring it for 4 months
(https://lkml.kernel.org/lkml/20200916045013.142179-1-ebigg...@kernel.org/T/#u).
drivers/char/random.c | 4 ++--
include/crypto/chacha.h | 9 +++--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index bbc5098b1a81f..4037a1e0fb748 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -809,7 +809,7 @@ static bool __init crng_init_try_arch_early(struct
crng_state *crng)
static void __maybe_unused crng_initialize_secondary(struct crng_state *crng)
{
- memcpy(>state[0], "expand 32-byte k", 16);
+ chacha_init_consts(crng->state);
_get_random_bytes(>state[4], sizeof(__u32) * 12);
crng_init_try_arch(crng);
crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
@@ -817,7 +817,7 @@ static void __maybe_unused crng_initialize_secondary(struct
crng_state *crng)
static void __init crng_initialize_primary(struct crng_state *crng)
{
- memcpy(>state[0], "expand 32-byte k", 16);
+ chacha_init_consts(crng->state);
_extract_entropy(_pool, >state[4], sizeof(__u32) * 12, 0);
if (crng_init_try_arch_early(crng) && trust_cpu) {
invalidate_batched_entropy();
diff --git a/include/crypto/chacha.h b/include/crypto/chacha.h
index 3a1c72fdb7cf5..dabaee6987186 100644
--- a/include/crypto/chacha.h
+++ b/include/crypto/chacha.h
@@ -47,13 +47,18 @@ static inline void hchacha_block(const u32 *state, u32
*out, int nrounds)
hchacha_block_generic(state, out, nrounds);
}
-void chacha_init_arch(u32 *state, const u32 *key, const u8 *iv);
-static inline void chacha_init_generic(u32 *state, const u32 *key, const u8
*iv)
+static inline void chacha_init_consts(u32 *state)
{
state[0] = 0x61707865; /* "expa" */
state[1] = 0x3320646e; /* "nd 3" */
state[2] = 0x79622d32; /* "2-by" */
state[3] = 0x6b206574; /* "te k" */
+}
+
+void chacha_init_arch(u32 *state, const u32 *key, const u8 *iv);
+static inline void chacha_init_generic(u32 *state, const u32 *key, const u8
*iv)
+{
+ chacha_init_consts(state);
state[4] = key[0];
state[5] = key[1];
state[6] = key[2];
--
2.30.0
Re: [PATCH 2/2] scsi: ufs: Remove unnecessary devm_kfree
On Mon, Jan 11, 2021 at 11:32:02PM +0100, Bean Huo wrote: > From: Bean Huo > > The memory allocated with devm_kzalloc() is freed automatically > no need to explicitly call devm_kfree. > > Signed-off-by: Bean Huo > --- > drivers/scsi/ufs/ufshcd-crypto.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd-crypto.c > b/drivers/scsi/ufs/ufshcd-crypto.c > index 07310b12a5dc..ec80ec83cf85 100644 > --- a/drivers/scsi/ufs/ufshcd-crypto.c > +++ b/drivers/scsi/ufs/ufshcd-crypto.c > @@ -182,7 +182,7 @@ int ufshcd_hba_init_crypto_capabilities(struct ufs_hba > *hba) > err = blk_ksm_init(>ksm, > hba->crypto_capabilities.config_count + 1); > if (err) > - goto out_free_caps; > + goto out; > > hba->ksm.ksm_ll_ops = ufshcd_ksm_ops; > /* UFS only supports 8 bytes for any DUN */ > @@ -208,8 +208,6 @@ int ufshcd_hba_init_crypto_capabilities(struct ufs_hba > *hba) > > return 0; > > -out_free_caps: > - devm_kfree(hba->dev, hba->crypto_cap_array); > out: > /* Indicate that init failed by clearing UFSHCD_CAP_CRYPTO */ > hba->caps &= ~UFSHCD_CAP_CRYPTO; Looks fine, feel free to add: Reviewed-by: Eric Biggers I think this was here to free the memory in the case where the crypto support gets disabled but the UFS host initialization still continues, so that the space wouldn't be wasted. But that's not what happens, as this is only reached on ENOMEM which is a fatal error. - Eric
Re: Malicious fs images was Re: ext4 regression in v5.9-rc2 from e7bfb5c9bb3d on ro fs with overlapped bitmaps
On Mon, Jan 11, 2021 at 10:51:20AM -0800, Darrick J. Wong wrote: > On Sun, Jan 10, 2021 at 07:41:02PM +0100, Pavel Machek wrote: > > Hi! > > > > On Fri 2020-10-09 10:37:32, Theodore Y. Ts'o wrote: > > > On Thu, Oct 08, 2020 at 03:22:59PM -0700, Josh Triplett wrote: > > > > > > > > I wasn't trying to make a *new* general principle or policy. I was under > > > > the impression that this *was* the policy, because it never occurred to > > > > me that it could be otherwise. It seemed like a natural aspect of the > > > > kernel/userspace boundary, to the point that the idea of it *not* being > > > > part of the kernel's stability guarantees didn't cross my mind. > > > > > > >From our perspective (and Darrick and I discussed this on this week's > > > ext4 video conference, so it represents the ext4 and xfs maintainer's > > > position) is that the file system format is different. First, the > > > on-disk format is not an ABI, and it is several orders more complex > > > than a system call interface. Second, we make no guarantees about > > > what the file system created by malicious tools will do. For example, > > > XFS developers reject bug reports from file system fuzzers, because > > My recollection of this is quite different -- sybot was sending multiple > zeroday exploits per week to the public xfs list, and nobody at Google > was helping us to /fix/ those bugs.Each report took hours of developer > time to extract the malicious image (because Dmitry couldn't figure out > how to add that ability to syzbot) and syscall sequence from the > reproducer program, plus whatever time it took to craft a patch, test > it, and push it through review. > > Dave and I complained to Dmitry about how the community had zero input > into the rate at which syzbot was allowed to look for xfs bugs. Nobody > at Google would commit to helping fix any of the XFS bugs, and Dmitry > would not give us better choices than (a) Google AI continuing to create > zerodays and leaving the community to clean up the mess, or (b) shutting > off syzbot entirely. At the time I said I would accept letting syzbot > run against xfs until it finds something, and turning it off until we > resolve the issue. That wasn't acceptable, because (I guess) nobody at > Google wants to put /any/ staff time into XFS at all. > > TLDR: XFS /does/ accept bug reports about fuzzed and broken images. > What we don't want is make-work Google AIs spraying zeroday code in > public places and the community developers having to clean up the mess. syzkaller is an open source project that implements a coverage-guided fuzzer for multiple operating system kernels; it's not "Google AI". Anyone can run syzkaller (either by itself, or as part of a syzbot instance) and find the same bugs. - Eric
Re: Re: [PATCH] evm: Fix memleak in init_desc
On Sun, Jan 10, 2021 at 01:27:09PM +0800, dinghao@zju.edu.cn wrote:
> > On Sat, Jan 09, 2021 at 07:33:05PM +0800, Dinghao Liu wrote:
> > > When kmalloc() fails, tmp_tfm allocated by
> > > crypto_alloc_shash() has not been freed, which
> > > leads to memleak.
> > >
> > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> > > Signed-off-by: Dinghao Liu
> > > ---
> > > security/integrity/evm/evm_crypto.c | 9 +++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/integrity/evm/evm_crypto.c
> > > b/security/integrity/evm/evm_crypto.c
> > > index 168c3b78ac47..39fb31a638ac 100644
> > > --- a/security/integrity/evm/evm_crypto.c
> > > +++ b/security/integrity/evm/evm_crypto.c
> > > @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> > > hash_algo)
> > > {
> > > long rc;
> > > const char *algo;
> > > - struct crypto_shash **tfm, *tmp_tfm;
> > > + struct crypto_shash **tfm, *tmp_tfm = NULL;
> > > struct shash_desc *desc;
> > >
> > > if (type == EVM_XATTR_HMAC) {
> > > @@ -118,13 +118,18 @@ static struct shash_desc *init_desc(char type,
> > > uint8_t hash_algo)
> > > alloc:
> > > desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > > GFP_KERNEL);
> > > - if (!desc)
> > > + if (!desc) {
> > > + if (tmp_tfm)
> > > + crypto_free_shash(tmp_tfm);
> > > return ERR_PTR(-ENOMEM);
> > > + }
> > >
> > > desc->tfm = *tfm;
> > >
> > > rc = crypto_shash_init(desc);
> > > if (rc) {
> > > + if (tmp_tfm)
> > > + crypto_free_shash(tmp_tfm);
> > > kfree(desc);
> > > return ERR_PTR(rc);
> > > }
> >
> > There's no need to check for NULL before calling crypto_free_shash().
> >
>
> I find there is a crypto_shash_tfm() in the definition of
> crypto_free_shash(). Will this lead to null pointer dereference
> when we use it to free a NULL pointer?
>
No. It does >base, not tfm->base.
- Eric
Re: KMSAN: uninit-value in __crypto_memneq (2)
+Jason, since this looks WireGuard-related. On Sat, Jan 09, 2021 at 05:05:24AM -0800, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit:73d62e81 kmsan: random: prevent boot-time reports in _mix_.. > git tree: https://github.com/google/kmsan.git master > console output: https://syzkaller.appspot.com/x/log.txt?x=142ab9c0d0 > kernel config: https://syzkaller.appspot.com/x/.config?x=2cdf4151c9653e32 > dashboard link: https://syzkaller.appspot.com/bug?extid=e0f501056b282add58a6 > compiler: clang version 11.0.0 > (https://github.com/llvm/llvm-project.git > ca2dcbd030eadbf0aa9b660efe864ff08af6e18b) > > Unfortunately, I don't have any reproducer for this issue yet. > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+e0f501056b282add5...@syzkaller.appspotmail.com > > = > BUG: KMSAN: uninit-value in __crypto_memneq_16 crypto/memneq.c:99 [inline] > BUG: KMSAN: uninit-value in __crypto_memneq+0x42c/0x470 crypto/memneq.c:161 > CPU: 0 PID: 20526 Comm: kworker/0:3 Not tainted 5.10.0-rc4-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > Workqueue: wg-crypt-wg1 wg_packet_decrypt_worker > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x21c/0x280 lib/dump_stack.c:118 > kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:118 > __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197 > __crypto_memneq_16 crypto/memneq.c:99 [inline] > __crypto_memneq+0x42c/0x470 crypto/memneq.c:161 > crypto_memneq include/crypto/algapi.h:277 [inline] > chacha20poly1305_crypt_sg_inplace+0x1662/0x1cd0 > lib/crypto/chacha20poly1305.c:311 > chacha20poly1305_decrypt_sg_inplace+0x179/0x1d0 > lib/crypto/chacha20poly1305.c:351 > decrypt_packet drivers/net/wireguard/receive.c:284 [inline] > wg_packet_decrypt_worker+0x9cf/0x17d0 drivers/net/wireguard/receive.c:509 > process_one_work+0x121c/0x1fc0 kernel/workqueue.c:2272 > worker_thread+0x10cc/0x2740 kernel/workqueue.c:2418 > kthread+0x51c/0x560 kernel/kthread.c:292 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 > > Uninit was stored to memory at: > kmsan_save_stack_with_flags mm/kmsan/kmsan.c:121 [inline] > kmsan_internal_chain_origin+0xad/0x130 mm/kmsan/kmsan.c:289 > __msan_chain_origin+0x57/0xa0 mm/kmsan/kmsan_instr.c:147 > put_unaligned_le64 include/linux/unaligned/access_ok.h:50 [inline] > poly1305_core_emit+0x625/0x6a0 lib/crypto/poly1305-donna64.c:182 > poly1305_final_generic+0xe2/0x280 lib/crypto/poly1305.c:71 > poly1305_final include/crypto/poly1305.h:94 [inline] > chacha20poly1305_crypt_sg_inplace+0x15cf/0x1cd0 > lib/crypto/chacha20poly1305.c:310 > chacha20poly1305_decrypt_sg_inplace+0x179/0x1d0 > lib/crypto/chacha20poly1305.c:351 > decrypt_packet drivers/net/wireguard/receive.c:284 [inline] > wg_packet_decrypt_worker+0x9cf/0x17d0 drivers/net/wireguard/receive.c:509 > process_one_work+0x121c/0x1fc0 kernel/workqueue.c:2272 > worker_thread+0x10cc/0x2740 kernel/workqueue.c:2418 > kthread+0x51c/0x560 kernel/kthread.c:292 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 > > Uninit was stored to memory at: > kmsan_save_stack_with_flags mm/kmsan/kmsan.c:121 [inline] > kmsan_internal_chain_origin+0xad/0x130 mm/kmsan/kmsan.c:289 > __msan_chain_origin+0x57/0xa0 mm/kmsan/kmsan_instr.c:147 > poly1305_core_blocks+0x8f4/0x940 lib/crypto/poly1305-donna64.c:107 > poly1305_update_generic+0x1a7/0x5a0 lib/crypto/poly1305.c:49 > poly1305_update include/crypto/poly1305.h:83 [inline] > chacha20poly1305_crypt_sg_inplace+0x1496/0x1cd0 > lib/crypto/chacha20poly1305.c:302 > chacha20poly1305_decrypt_sg_inplace+0x179/0x1d0 > lib/crypto/chacha20poly1305.c:351 > decrypt_packet drivers/net/wireguard/receive.c:284 [inline] > wg_packet_decrypt_worker+0x9cf/0x17d0 drivers/net/wireguard/receive.c:509 > process_one_work+0x121c/0x1fc0 kernel/workqueue.c:2272 > worker_thread+0x10cc/0x2740 kernel/workqueue.c:2418 > kthread+0x51c/0x560 kernel/kthread.c:292 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 > > Uninit was stored to memory at: > kmsan_save_stack_with_flags mm/kmsan/kmsan.c:121 [inline] > kmsan_internal_chain_origin+0xad/0x130 mm/kmsan/kmsan.c:289 > __msan_chain_origin+0x57/0xa0 mm/kmsan/kmsan_instr.c:147 > poly1305_core_blocks+0x8f4/0x940 lib/crypto/poly1305-donna64.c:107 > poly1305_update_generic+0x1a7/0x5a0 lib/crypto/poly1305.c:49 > poly1305_update include/crypto/poly1305.h:83 [inline] > chacha20poly1305_crypt_sg_inplace+0xb4d/0x1cd0 > lib/crypto/chacha20poly1305.c:263 > chacha20poly1305_decrypt_sg_inplace+0x179/0x1d0 > lib/crypto/chacha20poly1305.c:351 > decrypt_packet drivers/net/wireguard/receive.c:284 [inline] > wg_packet_decrypt_worker+0x9cf/0x17d0 drivers/net/wireguard/receive.c:509 > process_one_work+0x121c/0x1fc0 kernel/workqueue.c:2272 > worker_thread+0x10cc/0x2740 kernel/workqueue.c:2418
Re: [PATCH] evm: Fix memleak in init_desc
On Sat, Jan 09, 2021 at 07:33:05PM +0800, Dinghao Liu wrote:
> When kmalloc() fails, tmp_tfm allocated by
> crypto_alloc_shash() has not been freed, which
> leads to memleak.
>
> Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> Signed-off-by: Dinghao Liu
> ---
> security/integrity/evm/evm_crypto.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 168c3b78ac47..39fb31a638ac 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
> {
> long rc;
> const char *algo;
> - struct crypto_shash **tfm, *tmp_tfm;
> + struct crypto_shash **tfm, *tmp_tfm = NULL;
> struct shash_desc *desc;
>
> if (type == EVM_XATTR_HMAC) {
> @@ -118,13 +118,18 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
> alloc:
> desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> GFP_KERNEL);
> - if (!desc)
> + if (!desc) {
> + if (tmp_tfm)
> + crypto_free_shash(tmp_tfm);
> return ERR_PTR(-ENOMEM);
> + }
>
> desc->tfm = *tfm;
>
> rc = crypto_shash_init(desc);
> if (rc) {
> + if (tmp_tfm)
> + crypto_free_shash(tmp_tfm);
> kfree(desc);
> return ERR_PTR(rc);
> }
There's no need to check for NULL before calling crypto_free_shash().
- Eric
Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues
On Thu, Jan 07, 2021 at 10:14:46PM +, Russell King - ARM Linux admin wrote:
> On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o wrote:
> > >
> > > On Thu, Jan 07, 2021 at 01:37:47PM +, Russell King - ARM Linux admin
> > > wrote:
> > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > > them in my git history.
> > > >
> > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > or just for aarch64?
> > >
> > > Russell, Arnd, thanks so much for tracking down the root cause of the
> > > bug!
> >
> > There is one more thing that I wondered about when looking through
> > the ext4 code: Should it just call the crc32c_le() function directly
> > instead of going through the crypto layer? It seems that with Ard's
> > rework from 2018, that can just call the underlying architecture specific
> > implementation anyway.
>
> Yes, I've been wondering about that too. To me, it looks like the
> ext4 code performs a layering violation by going "under the covers"
> - there are accessor functions to set the CRC and retrieve it. ext4
> instead just makes the assumption that the CRC value is stored after
> struct shash_desc. Especially as the crypto/crc32c code references
> the value using:
>
> struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
>
> Not even crypto drivers are allowed to assume that desc+1 is where
> the CRC is stored.
It violates how the shash API is meant to be used in general, but there is a
test that enforces that the shash_desc_ctx for crc32c must be just the single
u32 crc value. See alg_test_crc32c() in crypto/testmgr.c. So it's apparently
intended to work.
>
> However, struct shash_desc is already 128 bytes in size on aarch64,
Ard Biesheuvel recently sent a patch to reduce the alignment of struct
shash_desc to ARCH_SLAB_MINALIGN
(https://lkml.kernel.org/linux-crypto/20210107124128.19791-1-a...@kernel.org/),
since apparently most of the bloat is from alignment for DMA, which isn't
necessary. I think that reduces the size by a lot on arm64.
> and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
> being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
> another bug to me!)
Are you referring to the '2 * sizeof(struct shash_desc)' rather than just
'sizeof(struct shash_desc)'? As mentioned in the comment above
HASH_MAX_DESCSIZE, there can be a nested shash_desc due to HMAC.
So I believe the value is correct.
> So, I agree with you wrt crc32c_le(), especially as it would be more
> efficient, and as the use of crc32c is already hard coded in the ext4
> code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
> the fixed-size structure in ext4_chksum().
>
> However, it's ultimately up to the ext4 maintainers to decide.
As I mentioned in my other response, crc32c_le() isn't a proper library API
(like some of the newer lib/crypto/ stuff) but rather just a wrapper for the
shash API, and it doesn't handle modules being dynamically loaded/unloaded.
So switching to it may cause a performance regression.
What I'd recommend is making crc32c_le() able to call architecture-speccific
implementations directly, similar to blake2s() and chacha20() in lib/crypto/.
Then there would be no concern about when modules get loaded, etc...
- Eric
Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues
On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote: > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o wrote: > > > > On Thu, Jan 07, 2021 at 01:37:47PM +, Russell King - ARM Linux admin > > wrote: > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see > > > > them in my git history. > > > > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1 > > > or just for aarch64? > > > > Russell, Arnd, thanks so much for tracking down the root cause of the > > bug! > > There is one more thing that I wondered about when looking through > the ext4 code: Should it just call the crc32c_le() function directly > instead of going through the crypto layer? It seems that with Ard's > rework from 2018, that can just call the underlying architecture specific > implementation anyway. > It looks like that would work, although note that crc32c_le() uses the shash API too, so it isn't any more "direct" than what ext4 does now. Also, a potential issue is that the implementation of crc32c that crc32c_le() uses might be chosen too early if the architecture-specific implementation of crc32c is compiled as a module (e.g. crc32c-intel.ko). There are two ways this could be fixed -- either by making it a proper library API like blake2s() that can call the architecture-specific code directly, or by reconfiguring things when a new crypto module is loaded (like what lib/crc-t10dif.c does). Until one of those is done, switching to crc32c_le() might cause performance regressions. - Eric
Re: [PATCH 3/5] crypto: add RFC5869 HKDF
On Thu, Jan 07, 2021 at 08:53:15AM +0100, Stephan Mueller wrote: > > > > > RFC5869 > > > allows two optional parameters to be provided to the extract operation: > > > the salt and additional information. Both are to be provided with the > > > seed parameter where the salt is the first entry of the seed parameter > > > and all subsequent entries are handled as additional information. If > > > the caller intends to invoke the HKDF without salt, it has to provide a > > > NULL/0 entry as first entry in seed. > > > > Where does "additional information" for extract come from? RFC 5869 has: > > > > HKDF-Extract(salt, IKM) -> PRK > > > > Inputs: > > salt optional salt value (a non-secret random value); > > if not provided, it is set to a string of HashLen > > zeros. > > IKM input keying material > > > > There's no "additional information". > > I used the terminology from SP800-108. I will update the description > accordingly. For HKDF, it would be better to stick to the terminology used in RFC 5869 (https://tools.ietf.org/html/rfc5869), as generally that's what people are most familiar with for HKDF. It also matches the HKDF paper (https://eprint.iacr.org/2010/264.pdf) more closely. - Eric
Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API
On Thu, Jan 07, 2021 at 08:49:52AM +0100, Stephan Mueller wrote: > > > -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, > > > +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key, > > > unsigned int master_key_size); > > > > It shouldn't be necessary to remove const here. > > Unfortunately it is when adding the pointer to struct kvec > > > > > > > > /* > > > @@ -323,7 +323,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const > > > u8 *master_key, > > > #define HKDF_CONTEXT_INODE_HASH_KEY7 /* info= */ > > > > > > int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, > > > - const u8 *info, unsigned int infolen, > > > + u8 *info, unsigned int infolen, > > > u8 *okm, unsigned int okmlen); > > > > Likewise. In fact some callers rely on 'info' not being modified. > > Same here. If the HKDF API will have a quirk like this, it's better not to "leak" it into the prototypes of these fscrypt functions. Just add the needed casts in fscrypt_init_hkdf() and fscrypt_hkdf_expand(). > > > - err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk)); > > > + err = crypto_hkdf_setkey(hmac_tfm, seed, ARRAY_SIZE(seed)); > > > if (err) > > > goto err_free_tfm; > > > > It's weird that the salt and key have to be passed in a kvec. > > Why not just have normal function parameters like: > > > > int crypto_hkdf_setkey(struct crypto_shash *hmac_tfm, > > const u8 *key, size_t keysize, > > const u8 *salt, size_t saltsize); > > I wanted to have an identical interface for all types of KDFs to allow turning > them into a template eventually. For example, SP800-108 KDFs only have one > parameter. Hence the use of a kvec. But the API being provided is a library function specifically for HKDF. So there's no need to make it conform to some other API. - Eric
Re: [PATCH 3/5] crypto: add RFC5869 HKDF
On Mon, Jan 04, 2021 at 10:49:13PM +0100, Stephan Müller wrote: > RFC5869 specifies an extract and expand two-step key derivation > function. The HKDF implementation is provided as a service function that > operates on a caller-provided HMAC cipher handle. HMAC isn't a "cipher". > The extract function is invoked via the crypto_hkdf_setkey call. Any reason not to call this crypto_hkdf_extract(), to match the specification? > RFC5869 > allows two optional parameters to be provided to the extract operation: > the salt and additional information. Both are to be provided with the > seed parameter where the salt is the first entry of the seed parameter > and all subsequent entries are handled as additional information. If > the caller intends to invoke the HKDF without salt, it has to provide a > NULL/0 entry as first entry in seed. Where does "additional information" for extract come from? RFC 5869 has: HKDF-Extract(salt, IKM) -> PRK Inputs: salt optional salt value (a non-secret random value); if not provided, it is set to a string of HashLen zeros. IKM input keying material There's no "additional information". > > The expand function is invoked via the crypto_hkdf_generate and can be > invoked multiple times. This function allows the caller to provide a > context for the key derivation operation. As specified in RFC5869, it is > optional. In case such context is not provided, the caller must provide > NULL / 0 for the info / info_nvec parameters. Any reason not to call this crypto_hkdf_expand() to match the specification? - Eric
Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API
On Mon, Jan 04, 2021 at 10:50:49PM +0100, Stephan Müller wrote:
> As the kernel crypto API implements HKDF, replace the
> file-system-specific HKDF implementation with the generic HKDF
> implementation.
>
> Signed-off-by: Stephan Mueller
> ---
> fs/crypto/Kconfig | 2 +-
> fs/crypto/fscrypt_private.h | 4 +-
> fs/crypto/hkdf.c| 108 +---
> 3 files changed, 30 insertions(+), 84 deletions(-)
>
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index a5f5c30368a2..9450e958f1d1 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -2,7 +2,7 @@
> config FS_ENCRYPTION
> bool "FS Encryption (Per-file encryption)"
> select CRYPTO
> - select CRYPTO_HASH
> + select CRYPTO_HKDF
> select CRYPTO_SKCIPHER
> select CRYPTO_LIB_SHA256
> select KEYS
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 3fa965eb3336..0d6871838099 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -304,7 +304,7 @@ struct fscrypt_hkdf {
> struct crypto_shash *hmac_tfm;
> };
>
> -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
> +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key,
> unsigned int master_key_size);
It shouldn't be necessary to remove const here.
>
> /*
> @@ -323,7 +323,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8
> *master_key,
> #define HKDF_CONTEXT_INODE_HASH_KEY 7 /* info= */
>
> int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
> - const u8 *info, unsigned int infolen,
> + u8 *info, unsigned int infolen,
> u8 *okm, unsigned int okmlen);
Likewise. In fact some callers rely on 'info' not being modified.
> -/*
> + *
> * Compute HKDF-Extract using the given master key as the input keying
> material,
> * and prepare an HMAC transform object keyed by the resulting pseudorandom
> key.
> *
> * Afterwards, the keyed HMAC transform object can be used for HKDF-Expand
> many
> * times without having to recompute HKDF-Extract each time.
> */
> -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
> +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key,
> unsigned int master_key_size)
> {
> + /* HKDF-Extract (RFC 5869 section 2.2), unsalted */
> + const struct kvec seed[] = { {
> + .iov_base = NULL,
> + .iov_len = 0
> + }, {
> + .iov_base = master_key,
> + .iov_len = master_key_size
> + } };
> struct crypto_shash *hmac_tfm;
> - u8 prk[HKDF_HASHLEN];
> int err;
>
> hmac_tfm = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);
> @@ -74,16 +65,12 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8
> *master_key,
> return PTR_ERR(hmac_tfm);
> }
>
> - if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != sizeof(prk))) {
> + if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != HKDF_HASHLEN)) {
> err = -EINVAL;
> goto err_free_tfm;
> }
>
> - err = hkdf_extract(hmac_tfm, master_key, master_key_size, prk);
> - if (err)
> - goto err_free_tfm;
> -
> - err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk));
> + err = crypto_hkdf_setkey(hmac_tfm, seed, ARRAY_SIZE(seed));
> if (err)
> goto err_free_tfm;
It's weird that the salt and key have to be passed in a kvec.
Why not just have normal function parameters like:
int crypto_hkdf_setkey(struct crypto_shash *hmac_tfm,
const u8 *key, size_t keysize,
const u8 *salt, size_t saltsize);
> int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
> - const u8 *info, unsigned int infolen,
> + u8 *info, unsigned int infolen,
> u8 *okm, unsigned int okmlen)
> {
> - SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
> - u8 prefix[9];
> - unsigned int i;
> - int err;
> - const u8 *prev = NULL;
> - u8 counter = 1;
> - u8 tmp[HKDF_HASHLEN];
> -
> - if (WARN_ON(okmlen > 255 * HKDF_HASHLEN))
> - return -EINVAL;
> -
> - desc->tfm = hkdf->hmac_tfm;
> -
> - memcpy(prefix, "fscrypt\0", 8);
> - prefix[8] = context;
> -
> - for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
> + const struct kvec info_iov[] = { {
> + .iov_base = "fscrypt\0",
> + .iov_len = 8,
> + }, {
> + .iov_base = ,
> + .iov_len = 1,
> + }, {
> + .iov_base = info,
> + .iov_len = infolen,
> + } };
> + int err = crypto_hkdf_generate(hkdf->hmac_tfm,
> +info_iov, ARRAY_SIZE(info_iov),
> +okm,
Re: [PATCH 0/5] Add KDF implementations to crypto API
On Wed, Jan 06, 2021 at 10:59:24PM -0800, Eric Biggers wrote: > On Thu, Jan 07, 2021 at 07:37:05AM +0100, Stephan Mueller wrote: > > Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers: > > > On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote: > > > > The HKDF addition is used to replace the implementation in the > > > > filesystem > > > > crypto extension. This code was tested by using an EXT4 encrypted file > > > > system that was created and contains files written to by the current > > > > implementation. Using the new implementation a successful read of the > > > > existing files was possible and new files / directories were created > > > > and read successfully. These newly added file system objects could be > > > > successfully read using the current code. Yet if there is a test suite > > > > to validate whether the invokcation of the HKDF calculates the same > > > > result as the existing implementation, I would be happy to validate > > > > the implementation accordingly. > > > > > > See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests > > > for how to run the fscrypt tests. 'kvm-xfstests -c ext4 generic/582' > > > should > > > be > > > enough for this, though you could run all the tests if you want. > > > > I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my > > HKDF changes. I.e. the testing shows the same results for both kernels which > > seems to imply that my HKDF changes do not change the behavior. > > > > I get the following errors in both occasions - let me know if I should dig a > > bit more. > > The command you ran runs almost all xfstests with the test_dummy_encryption > mount option enabled, which is different from running the encryption tests -- > and in fact it skips the real encryption tests, so it doesn't test the > correctness of HKDF at all. It looks like you saw some unrelated test > failures. > Sorry if I wasn't clear -- by "all tests" I meant all encryption tests, i.e. > 'kvm-xfstests -c ext4 -g encrypt'. Also, even the single test generic/582 > should be sufficient to test HKDF, as I mentioned. > I just did it myself and the tests pass. - Eric
Re: [PATCH 0/5] Add KDF implementations to crypto API
On Thu, Jan 07, 2021 at 07:37:05AM +0100, Stephan Mueller wrote: > Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers: > > On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote: > > > The HKDF addition is used to replace the implementation in the filesystem > > > crypto extension. This code was tested by using an EXT4 encrypted file > > > system that was created and contains files written to by the current > > > implementation. Using the new implementation a successful read of the > > > existing files was possible and new files / directories were created > > > and read successfully. These newly added file system objects could be > > > successfully read using the current code. Yet if there is a test suite > > > to validate whether the invokcation of the HKDF calculates the same > > > result as the existing implementation, I would be happy to validate > > > the implementation accordingly. > > > > See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests > > for how to run the fscrypt tests. 'kvm-xfstests -c ext4 generic/582' should > > be > > enough for this, though you could run all the tests if you want. > > I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my > HKDF changes. I.e. the testing shows the same results for both kernels which > seems to imply that my HKDF changes do not change the behavior. > > I get the following errors in both occasions - let me know if I should dig a > bit more. The command you ran runs almost all xfstests with the test_dummy_encryption mount option enabled, which is different from running the encryption tests -- and in fact it skips the real encryption tests, so it doesn't test the correctness of HKDF at all. It looks like you saw some unrelated test failures. Sorry if I wasn't clear -- by "all tests" I meant all encryption tests, i.e. 'kvm-xfstests -c ext4 -g encrypt'. Also, even the single test generic/582 should be sufficient to test HKDF, as I mentioned. - Eric
Re: [PATCH 0/5] Add KDF implementations to crypto API
On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote: > The HKDF addition is used to replace the implementation in the filesystem > crypto extension. This code was tested by using an EXT4 encrypted file > system that was created and contains files written to by the current > implementation. Using the new implementation a successful read of the > existing files was possible and new files / directories were created > and read successfully. These newly added file system objects could be > successfully read using the current code. Yet if there is a test suite > to validate whether the invokcation of the HKDF calculates the same > result as the existing implementation, I would be happy to validate > the implementation accordingly. See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests for how to run the fscrypt tests. 'kvm-xfstests -c ext4 generic/582' should be enough for this, though you could run all the tests if you want. - Eric
Re: [PATCH] random: initialize ChaCha20 constants with correct endianness
On Fri, Nov 20, 2020 at 10:52:54AM -0800, Eric Biggers wrote: > On Mon, Oct 26, 2020 at 09:33:54AM -0700, Eric Biggers wrote: > > On Tue, Oct 06, 2020 at 08:51:45PM -0700, Eric Biggers wrote: > > > On Fri, Sep 18, 2020 at 02:57:05PM -0700, Eric Biggers wrote: > > > > On Fri, Sep 18, 2020 at 04:42:07PM -0400, Theodore Y. Ts'o wrote: > > > > > > Ted, any further feedback on this? Are you planning to apply this patch? > > > > > > - Eric > > > > Ping. > > Ping. Ping.
Re: [PATCH] random: remove dead code left over from blocking pool
On Fri, Nov 20, 2020 at 10:52:35AM -0800, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 09:34:03AM -0700, Eric Biggers wrote:
> > On Tue, Oct 06, 2020 at 08:50:58PM -0700, Eric Biggers wrote:
> > > On Tue, Sep 15, 2020 at 09:36:52PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers
> > > >
> > > > Remove some dead code that was left over following commit 90ea1c6436d2
> > > > ("random: remove the blocking pool").
> > > >
> > > > Signed-off-by: Eric Biggers
> > >
> > > Ping?
> >
> > Ping.
>
> Ping.
Ping.
Re: [PATCH] random: fix the RNDRESEEDCRNG ioctl
On Fri, Nov 20, 2020 at 10:52:14AM -0800, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 09:33:43AM -0700, Eric Biggers wrote:
> > On Tue, Oct 06, 2020 at 08:50:21PM -0700, Eric Biggers wrote:
> > > On Tue, Sep 15, 2020 at 09:19:08PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers
> > > >
> > > > The RNDRESEEDCRNG ioctl reseeds the primary_crng from itself, which
> > > > doesn't make sense. Reseed it from the input_pool instead.
> > > >
> > > > Fixes: d848e5f8e1eb ("random: add new ioctl RNDRESEEDCRNG")
> > > > Cc: sta...@vger.kernel.org
> > > > Signed-off-by: Eric Biggers
> > >
> > > Ping?
> >
> > Ping.
>
> Ping.
Ping.
Re: [PATCH v3] vfs: don't unnecessarily clone write access for writable fds
On Tue, Sep 22, 2020 at 09:44:18AM -0700, Eric Biggers wrote: > From: Eric Biggers > > There's no need for mnt_want_write_file() to increment mnt_writers when > the file is already open for writing, provided that > mnt_drop_write_file() is changed to conditionally decrement it. > > We seem to have ended up in the current situation because > mnt_want_write_file() used to be paired with mnt_drop_write(), due to > mnt_drop_write_file() not having been added yet. So originally > mnt_want_write_file() had to always increment mnt_writers. > > But later mnt_drop_write_file() was added, and all callers of > mnt_want_write_file() were paired with it. This makes the compatibility > between mnt_want_write_file() and mnt_drop_write() no longer necessary. > > Therefore, make __mnt_want_write_file() and __mnt_drop_write_file() skip > incrementing mnt_writers on files already open for writing. This > removes the only caller of mnt_clone_write(), so remove that too. > > Signed-off-by: Eric Biggers > --- > > v3: added note to porting file. > v2: keep the check for emergency r/o remounts. > > Documentation/filesystems/porting.rst | 7 > fs/namespace.c| 53 ++- > include/linux/mount.h | 1 - > 3 files changed, 27 insertions(+), 34 deletions(-) Ping.
Re: [f2fs-dev] [PATCH AUTOSEL 5.10 10/31] f2fs: Handle casefolding with Encryption
On Wed, Dec 30, 2020 at 08:02:52AM -0500, Sasha Levin wrote: > From: Daniel Rosenberg > > [ Upstream commit 7ad08a58bf67594057362e45cbddd3e27e53e557 ] > > Expand f2fs's casefolding support to include encrypted directories. To > index casefolded+encrypted directories, we use the SipHash of the > casefolded name, keyed by a key derived from the directory's fscrypt > master key. This ensures that the dirhash doesn't leak information > about the plaintext filenames. > > Encryption keys are unavailable during roll-forward recovery, so we > can't compute the dirhash when recovering a new dentry in an encrypted + > casefolded directory. To avoid having to force a checkpoint when a new > file is fsync'ed, store the dirhash on-disk appended to i_name. > > This patch incorporates work by Eric Biggers > and Jaegeuk Kim . > > Co-developed-by: Eric Biggers > Signed-off-by: Eric Biggers > Signed-off-by: Daniel Rosenberg > Reviewed-by: Eric Biggers > Signed-off-by: Jaegeuk Kim > Signed-off-by: Sasha Levin Please don't backport this to the LTS kernels. This is a new feature, not a fix, and you missed prerequisite patches... - Eric
Re: [PATCH 0/2] crypto: x86/aes-ni-xts - recover and improve performance
On Tue, Dec 22, 2020 at 05:06:27PM +0100, Ard Biesheuvel wrote:
> The AES-NI implementation of XTS was impacted significantly by the retpoline
> changes, which is due to the fact that both its asm helper and the chaining
> mode glue library use indirect calls for processing small quantitities of
> data
>
> So let's fix this, by:
> - creating a minimal, backportable fix that recovers most of the performance,
> by reducing the number of indirect calls substantially;
> - for future releases, rewrite the XTS implementation completely, and replace
> the glue helper with a core asm routine that is more flexible, making the C
> code wrapper much more straight-forward.
>
> This results in a substantial performance improvement: around ~2x for 1k and
> 4k blocks, and more than 3x for ~1k blocks that require ciphertext stealing
> (benchmarked using tcrypt using 1420 byte blocks - full results below)
>
> It also allows us to enable the same driver for i386.
>
> Cc: Megha Dey
> Cc: Eric Biggers
> Cc: Herbert Xu
>
> Ard Biesheuvel (2):
> crypto: x86/aes-ni-xts - use direct calls to and 4-way stride
> crypto: x86/aes-ni-xts - rewrite and drop indirections via glue helper
>
> arch/x86/crypto/aesni-intel_asm.S | 353
> arch/x86/crypto/aesni-intel_glue.c | 230 +++--
> 2 files changed, 412 insertions(+), 171 deletions(-)
>
> --
> 2.17.1
>
> Benchmarked using tcrypt on a Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz.
Thanks for doing this! I didn't realize that there was such a big performance
regression here. Getting rid of these indirect calls looks like the right
approach; this all seems to have been written for a world where indirect calls
are much faster...
I did some quick benchmarks on Zen ("AMD Ryzen Threadripper 1950X 16-Core
Processor") with CONFIG_RETPOLINE=y and confirmed the speedup on 4096-byte
blocks is around 2x there too. (It's over 2x for AES-128-XTS and AES-192-XTS,
and a bit under 2x for AES-256-XTS. And most of the speedup comes from the
first patch.) Also, the extra self-tests are passing.
So feel free to add:
Tested-by: Eric Biggers # x86_64
Note that this patch series didn't apply cleanly, as it seems to depend on some
other patches you've sent out recently. So I actually tested your
"for-kernelci" branch instead of applying these directly.
- Eric
Re: [RFC V1 0/7] Introduce AVX512 optimized crypto algorithms
On Fri, Dec 18, 2020 at 01:10:57PM -0800, Megha Dey wrote: > Optimize crypto algorithms using VPCLMULQDQ and VAES AVX512 instructions > (first implemented on Intel's Icelake client and Xeon CPUs). > > These algorithms take advantage of the AVX512 registers to keep the CPU > busy and increase memory bandwidth utilization. They provide substantial > (2-10x) improvements over existing crypto algorithms when update data size > is greater than 128 bytes and do not have any significant impact when used > on small amounts of data. > > However, these algorithms may also incur a frequency penalty and cause > collateral damage to other workloads running on the same core(co-scheduled > threads). These frequency drops are also known as bin drops where 1 bin > drop is around 100MHz. With the SpecCPU and ffmpeg benchmark, a 0-1 bin > drop(0-100MHz) is observed on Icelake desktop and 0-2 bin drops (0-200Mhz) > are observed on the Icelake server. > Do these new algorithms all pass the self-tests, including the fuzz tests that are enabled when CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y? - Eric
