* [PATCH v4 8/8] integrity: Asymmetric digsig supports SM2-with-SM3 algorithm
From: Tianjia Zhang @ 2020-07-08 8:28 UTC (permalink / raw)
To: herbert, davem, dhowells, mcoquelin.stm32, alexandre.torgue,
jmorris, serge, nramas, tusharsu, zohar, gilad, pvanleeuwen
Cc: linux-crypto, linux-kernel, keyrings, linux-stm32,
linux-arm-kernel, linux-security-module, linux-integrity,
zhang.jia, tianjia.zhang
In-Reply-To: <20200708082818.5511-1-tianjia.zhang@linux.alibaba.com>
Asymmetric digsig supports SM2-with-SM3 algorithm combination,
so that IMA can also verify SM2's signature data.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
security/integrity/digsig_asymmetric.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 4e0d6778277e..9350fcfb9bf2 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -99,14 +99,22 @@ int asymmetric_verify(struct key *keyring, const char *sig,
memset(&pks, 0, sizeof(pks));
pks.hash_algo = hash_algo_name[hdr->hash_algo];
- if (hdr->hash_algo == HASH_ALGO_STREEBOG_256 ||
- hdr->hash_algo == HASH_ALGO_STREEBOG_512) {
+ switch (hdr->hash_algo) {
+ case HASH_ALGO_STREEBOG_256:
+ case HASH_ALGO_STREEBOG_512:
/* EC-RDSA and Streebog should go together. */
pks.pkey_algo = "ecrdsa";
pks.encoding = "raw";
- } else {
+ break;
+ case HASH_ALGO_SM3_256:
+ /* SM2 and SM3 should go together. */
+ pks.pkey_algo = "sm2";
+ pks.encoding = "raw";
+ break;
+ default:
pks.pkey_algo = "rsa";
pks.encoding = "pkcs1";
+ break;
}
pks.digest = (u8 *)data;
pks.digest_size = datalen;
--
2.17.1
^ permalink raw reply related
* [PATCH v4 7/8] X.509: support OSCCA sm2-with-sm3 certificate verification
From: Tianjia Zhang @ 2020-07-08 8:28 UTC (permalink / raw)
To: herbert, davem, dhowells, mcoquelin.stm32, alexandre.torgue,
jmorris, serge, nramas, tusharsu, zohar, gilad, pvanleeuwen
Cc: linux-crypto, linux-kernel, keyrings, linux-stm32,
linux-arm-kernel, linux-security-module, linux-integrity,
zhang.jia, tianjia.zhang
In-Reply-To: <20200708082818.5511-1-tianjia.zhang@linux.alibaba.com>
The digital certificate format based on SM2 crypto algorithm as
specified in GM/T 0015-2012. It was published by State Encryption
Management Bureau, China.
The method of generating Other User Information is defined as
ZA=H256(ENTLA || IDA || a || b || xG || yG || xA || yA), it also
specified in https://tools.ietf.org/html/draft-shen-sm2-ecdsa-02.
The x509 certificate supports sm2-with-sm3 type certificate
verification. Because certificate verification requires ZA
in addition to tbs data, ZA also depends on elliptic curve
parameters and public key data, so you need to access tbs in sig
and calculate ZA. Finally calculate the digest of the
signature and complete the verification work. The calculation
process of ZA is declared in specifications GM/T 0009-2012
and GM/T 0003.2-2012.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
crypto/asymmetric_keys/Makefile | 1 +
crypto/asymmetric_keys/public_key.c | 6 +++
crypto/asymmetric_keys/public_key_sm2.c | 57 ++++++++++++++++++++++++
crypto/asymmetric_keys/x509_public_key.c | 3 ++
include/crypto/public_key.h | 15 +++++++
5 files changed, 82 insertions(+)
create mode 100644 crypto/asymmetric_keys/public_key_sm2.c
diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile
index 28b91adba2ae..d499367dd253 100644
--- a/crypto/asymmetric_keys/Makefile
+++ b/crypto/asymmetric_keys/Makefile
@@ -11,6 +11,7 @@ asymmetric_keys-y := \
signature.o
obj-$(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
+obj-$(CONFIG_CRYPTO_SM2) += public_key_sm2.o
obj-$(CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE) += asym_tpm.o
#
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index d7f43d4ea925..6b7a6286d5fd 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -298,6 +298,12 @@ int public_key_verify_signature(const struct public_key *pkey,
if (ret)
goto error_free_key;
+ if (strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
+ ret = cert_sig_digest_update(sig, tfm);
+ if (ret)
+ goto error_free_key;
+ }
+
sg_init_table(src_sg, 2);
sg_set_buf(&src_sg[0], sig->s, sig->s_size);
sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
diff --git a/crypto/asymmetric_keys/public_key_sm2.c b/crypto/asymmetric_keys/public_key_sm2.c
new file mode 100644
index 000000000000..b3df5c615b72
--- /dev/null
+++ b/crypto/asymmetric_keys/public_key_sm2.c
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * asymmetric public-key algorithm for SM2-with-SM3 certificate
+ * as specified by OSCCA GM/T 0003.1-2012 -- 0003.5-2012 SM2 and
+ * described at https://tools.ietf.org/html/draft-shen-sm2-ecdsa-02
+ *
+ * Copyright (c) 2020, Alibaba Group.
+ * Authors: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
+ */
+
+#include <crypto/sm3_base.h>
+#include <crypto/sm2.h>
+#include "x509_parser.h"
+
+int cert_sig_digest_update(const struct public_key_signature *sig,
+ struct crypto_akcipher *tfm_pkey)
+{
+ struct crypto_shash *tfm;
+ struct shash_desc *desc;
+ size_t desc_size;
+ unsigned char dgst[SM3_DIGEST_SIZE];
+ int ret;
+
+ BUG_ON(!sig->data);
+
+ ret = sm2_compute_z_digest(tfm_pkey, SM2_DEFAULT_USERID,
+ SM2_DEFAULT_USERID_LEN, dgst);
+ if (ret)
+ return ret;
+
+ tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
+ if (IS_ERR(tfm))
+ return PTR_ERR(tfm);
+
+ desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+ desc = kzalloc(desc_size, GFP_KERNEL);
+ if (!desc)
+ goto error_free_tfm;
+
+ desc->tfm = tfm;
+
+ ret = crypto_shash_init(desc);
+ if (ret < 0)
+ goto error_free_desc;
+
+ ret = crypto_shash_update(desc, dgst, SM3_DIGEST_SIZE);
+ if (ret < 0)
+ goto error_free_desc;
+
+ ret = crypto_shash_finup(desc, sig->data, sig->data_size, sig->digest);
+
+error_free_desc:
+ kfree(desc);
+error_free_tfm:
+ crypto_free_shash(tfm);
+ return ret;
+}
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index d964cc82b69c..ae450eb8be14 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -30,6 +30,9 @@ int x509_get_sig_params(struct x509_certificate *cert)
pr_devel("==>%s()\n", __func__);
+ sig->data = cert->tbs;
+ sig->data_size = cert->tbs_size;
+
if (!cert->pub->pkey_algo)
cert->unsupported_key = true;
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 0588ef3bc6ff..861348dccd68 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -12,6 +12,7 @@
#include <linux/keyctl.h>
#include <linux/oid_registry.h>
+#include <crypto/akcipher.h>
/*
* Cryptographic data for the public-key subtype of the asymmetric key type.
@@ -44,6 +45,8 @@ struct public_key_signature {
const char *pkey_algo;
const char *hash_algo;
const char *encoding;
+ const void *data;
+ unsigned int data_size;
};
extern void public_key_signature_free(struct public_key_signature *sig);
@@ -81,4 +84,16 @@ extern int verify_signature(const struct key *,
int public_key_verify_signature(const struct public_key *pkey,
const struct public_key_signature *sig);
+#ifdef CONFIG_CRYPTO_SM2
+int cert_sig_digest_update(const struct public_key_signature *sig,
+ struct crypto_akcipher *tfm_pkey);
+#else
+static inline
+int cert_sig_digest_update(const struct public_key_signature *sig,
+ struct crypto_akcipher *tfm_pkey)
+{
+ return -ENOTSUPP;
+}
+#endif
+
#endif /* _LINUX_PUBLIC_KEY_H */
--
2.17.1
^ permalink raw reply related
* [PATCH v4 0/8] crpyto: introduce OSCCA certificate and SM2 asymmetric algorithm
From: Tianjia Zhang @ 2020-07-08 8:28 UTC (permalink / raw)
To: herbert, davem, dhowells, mcoquelin.stm32, alexandre.torgue,
jmorris, serge, nramas, tusharsu, zohar, gilad, pvanleeuwen
Cc: linux-crypto, linux-kernel, keyrings, linux-stm32,
linux-arm-kernel, linux-security-module, linux-integrity,
zhang.jia, tianjia.zhang
Hello all,
This new module implement the OSCCA certificate and SM2 public key
algorithm. It was published by State Encryption Management Bureau, China.
List of specifications for OSCCA certificate and SM2 elliptic curve
public key cryptography:
* GM/T 0003.1-2012
* GM/T 0003.2-2012
* GM/T 0003.3-2012
* GM/T 0003.4-2012
* GM/T 0003.5-2012
* GM/T 0015-2012
* GM/T 0009-2012
IETF: https://tools.ietf.org/html/draft-shen-sm2-ecdsa-02
oscca: http://www.oscca.gov.cn/sca/xxgk/2010-12/17/content_1002386.shtml
scctc: http://www.gmbz.org.cn/main/bzlb.html
These patchs add the OID object identifier defined by OSCCA. The
x509 certificate supports sm2-with-sm3 type certificate parsing
and verification.
The sm2 algorithm is based on libgcrypt's mpi implementation, and has
made some additions to the kernel's original mpi library, and added the
implementation of ec to better support elliptic curve-like algorithms.
sm2 has good support in both openssl and gnupg projects, and sm3 and sm4
of the OSCCA algorithm family have also been implemented in the kernel.
Among them, sm3 and sm4 have been well implemented in the kernel.
This group of patches has newly introduced sm2. In order to implement
sm2 more perfectly, I expanded the mpi library and introduced the
ec implementation of the mpi library as the basic algorithm. Compared
to the kernel's crypto/ecc.c, the implementation of mpi/ec.c is more
complete and elegant, sm2 is implemented based on these algorithms.
---
v4 changes:
1. Pass data directly when calculating sm2 certificate digest
2. rebase on mainline.
v3 changes:
1. integrity asymmetric digsig support sm2-with-sm3 algorithm.
2. remove unused sm2_set_priv_key().
3. rebase on mainline.
v2 changes:
1. simplify the sm2 algorithm and only retain the verify function.
2. extract the sm2 certificate code into a separate file.
Tianjia Zhang (8):
crypto: sm3 - export crypto_sm3_final function
lib/mpi: Extend the MPI library
lib/mpi: Introduce ec implementation to MPI library
crypto: sm2 - introduce OSCCA SM2 asymmetric cipher algorithm
crypto: testmgr - support test with different ciphertext per
encryption
X.509: support OSCCA certificate parse
X.509: support OSCCA sm2-with-sm3 certificate verification
integrity: Asymmetric digsig supports SM2-with-SM3 algorithm
crypto/Kconfig | 17 +
crypto/Makefile | 8 +
crypto/asymmetric_keys/Makefile | 1 +
crypto/asymmetric_keys/public_key.c | 6 +
crypto/asymmetric_keys/public_key_sm2.c | 57 +
crypto/asymmetric_keys/x509_cert_parser.c | 14 +-
crypto/asymmetric_keys/x509_public_key.c | 3 +
crypto/sm2.c | 473 +++++++
crypto/sm2signature.asn1 | 4 +
crypto/sm3_generic.c | 7 +-
crypto/testmgr.c | 7 +-
include/crypto/public_key.h | 15 +
include/crypto/sm2.h | 25 +
include/crypto/sm3.h | 2 +
include/linux/mpi.h | 193 +++
include/linux/oid_registry.h | 6 +
lib/mpi/Makefile | 6 +
lib/mpi/ec.c | 1538 +++++++++++++++++++++
lib/mpi/mpi-add.c | 207 +++
lib/mpi/mpi-bit.c | 251 ++++
lib/mpi/mpi-cmp.c | 46 +-
lib/mpi/mpi-div.c | 259 ++++
lib/mpi/mpi-internal.h | 53 +
lib/mpi/mpi-inv.c | 143 ++
lib/mpi/mpi-mod.c | 155 +++
lib/mpi/mpi-mul.c | 166 +++
lib/mpi/mpicoder.c | 336 +++++
lib/mpi/mpih-div.c | 294 ++++
lib/mpi/mpih-mul.c | 25 +
lib/mpi/mpiutil.c | 204 +++
security/integrity/digsig_asymmetric.c | 14 +-
31 files changed, 4517 insertions(+), 18 deletions(-)
create mode 100644 crypto/asymmetric_keys/public_key_sm2.c
create mode 100644 crypto/sm2.c
create mode 100644 crypto/sm2signature.asn1
create mode 100644 include/crypto/sm2.h
create mode 100644 lib/mpi/ec.c
create mode 100644 lib/mpi/mpi-add.c
create mode 100644 lib/mpi/mpi-div.c
create mode 100644 lib/mpi/mpi-inv.c
create mode 100644 lib/mpi/mpi-mod.c
create mode 100644 lib/mpi/mpi-mul.c
--
2.17.1
^ permalink raw reply
* [PATCH v4 5/8] crypto: testmgr - support test with different ciphertext per encryption
From: Tianjia Zhang @ 2020-07-08 8:28 UTC (permalink / raw)
To: herbert, davem, dhowells, mcoquelin.stm32, alexandre.torgue,
jmorris, serge, nramas, tusharsu, zohar, gilad, pvanleeuwen
Cc: linux-crypto, linux-kernel, keyrings, linux-stm32,
linux-arm-kernel, linux-security-module, linux-integrity,
zhang.jia, tianjia.zhang
In-Reply-To: <20200708082818.5511-1-tianjia.zhang@linux.alibaba.com>
Some asymmetric algorithms will get different ciphertext after
each encryption, such as SM2, and let testmgr support the testing
of such algorithms.
In struct akcipher_testvec, set c and c_size to be empty, skip
the comparison of the ciphertext, and compare the decrypted
plaintext with m to achieve the test purpose.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
crypto/testmgr.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 6863f911fcee..0dc94461c437 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -4025,7 +4025,7 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
pr_err("alg: akcipher: %s test failed. err %d\n", op, err);
goto free_all;
}
- if (!vecs->siggen_sigver_test) {
+ if (!vecs->siggen_sigver_test && c) {
if (req->dst_len != c_size) {
pr_err("alg: akcipher: %s test failed. Invalid output len\n",
op);
@@ -4056,6 +4056,11 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
goto free_all;
}
+ if (!vecs->siggen_sigver_test && !c) {
+ c = outbuf_enc;
+ c_size = req->dst_len;
+ }
+
op = vecs->siggen_sigver_test ? "sign" : "decrypt";
if (WARN_ON(c_size > PAGE_SIZE))
goto free_all;
--
2.17.1
^ permalink raw reply related
* [PATCH v4 1/8] crypto: sm3 - export crypto_sm3_final function
From: Tianjia Zhang @ 2020-07-08 8:28 UTC (permalink / raw)
To: herbert, davem, dhowells, mcoquelin.stm32, alexandre.torgue,
jmorris, serge, nramas, tusharsu, zohar, gilad, pvanleeuwen
Cc: linux-crypto, linux-kernel, keyrings, linux-stm32,
linux-arm-kernel, linux-security-module, linux-integrity,
zhang.jia, tianjia.zhang
In-Reply-To: <20200708082818.5511-1-tianjia.zhang@linux.alibaba.com>
Both crypto_sm3_update and crypto_sm3_finup have been
exported, exporting crypto_sm3_final, to avoid having to
use crypto_sm3_finup(desc, NULL, 0, dgst) to calculate
the hash in some cases.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
crypto/sm3_generic.c | 7 ++++---
include/crypto/sm3.h | 2 ++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/crypto/sm3_generic.c b/crypto/sm3_generic.c
index 3468975215ca..193c4584bd00 100644
--- a/crypto/sm3_generic.c
+++ b/crypto/sm3_generic.c
@@ -149,17 +149,18 @@ int crypto_sm3_update(struct shash_desc *desc, const u8 *data,
}
EXPORT_SYMBOL(crypto_sm3_update);
-static int sm3_final(struct shash_desc *desc, u8 *out)
+int crypto_sm3_final(struct shash_desc *desc, u8 *out)
{
sm3_base_do_finalize(desc, sm3_generic_block_fn);
return sm3_base_finish(desc, out);
}
+EXPORT_SYMBOL(crypto_sm3_final);
int crypto_sm3_finup(struct shash_desc *desc, const u8 *data,
unsigned int len, u8 *hash)
{
sm3_base_do_update(desc, data, len, sm3_generic_block_fn);
- return sm3_final(desc, hash);
+ return crypto_sm3_final(desc, hash);
}
EXPORT_SYMBOL(crypto_sm3_finup);
@@ -167,7 +168,7 @@ static struct shash_alg sm3_alg = {
.digestsize = SM3_DIGEST_SIZE,
.init = sm3_base_init,
.update = crypto_sm3_update,
- .final = sm3_final,
+ .final = crypto_sm3_final,
.finup = crypto_sm3_finup,
.descsize = sizeof(struct sm3_state),
.base = {
diff --git a/include/crypto/sm3.h b/include/crypto/sm3.h
index 1438942dc773..42ea21289ba9 100644
--- a/include/crypto/sm3.h
+++ b/include/crypto/sm3.h
@@ -35,6 +35,8 @@ struct shash_desc;
extern int crypto_sm3_update(struct shash_desc *desc, const u8 *data,
unsigned int len);
+extern int crypto_sm3_final(struct shash_desc *desc, u8 *out);
+
extern int crypto_sm3_finup(struct shash_desc *desc, const u8 *data,
unsigned int len, u8 *hash);
#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v19 09/12] arch: Wire up landlock() syscall
From: Mickaël Salaün @ 2020-07-08 8:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Al Viro, Andy Lutomirski,
Anton Ivanov, Casey Schaufler, James Morris, Jann Horn, Jeff Dike,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Richard Weinberger, Serge E . Hallyn,
Shuah Khan, Vincent Dagonneau, Kernel Hardening, Linux API,
linux-arch, open list:DOCUMENTATION, Linux FS-devel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, LSM List,
the arch/x86 maintainers
In-Reply-To: <CAK8P3a3Mf_+-MY5kdeY7sqwUgCUi=PksWz1pGDy+o0ZfgF93Zw@mail.gmail.com>
On 08/07/2020 09:47, Arnd Bergmann wrote:
> On Wed, Jul 8, 2020 at 9:31 AM Mickaël Salaün <mic@digikod.net> wrote:
>> On 08/07/2020 09:22, Arnd Bergmann wrote:
>>> On Tue, Jul 7, 2020 at 8:10 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>>> index f4a01305d9a6..a63a411a74d5 100644
>>>> --- a/include/uapi/asm-generic/unistd.h
>>>> +++ b/include/uapi/asm-generic/unistd.h
>>
>> OK, I'll rebase the next series on linux-next.
>
> Just change the number to the next free one, without actually rebasing.
> It's always a bit messy to have multiple syscalls added, but I think that
> causes the least confusion.
OK, but this will lead to two merge conflicts: patch 8 (asmlinkage) and
patch 9 (tbl files).
Do you want me to update the tools/perf/arch/*.tbl too?
^ permalink raw reply
* Re: [PATCH v19 09/12] arch: Wire up landlock() syscall
From: Arnd Bergmann @ 2020-07-08 7:47 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel@vger.kernel.org, Al Viro, Andy Lutomirski,
Anton Ivanov, Casey Schaufler, James Morris, Jann Horn, Jeff Dike,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Richard Weinberger, Serge E . Hallyn,
Shuah Khan, Vincent Dagonneau, Kernel Hardening, Linux API,
linux-arch, open list:DOCUMENTATION, Linux FS-devel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, LSM List,
the arch/x86 maintainers
In-Reply-To: <8d2dab03-289e-2872-db66-ce80ce5c189f@digikod.net>
On Wed, Jul 8, 2020 at 9:31 AM Mickaël Salaün <mic@digikod.net> wrote:
> On 08/07/2020 09:22, Arnd Bergmann wrote:
> > On Tue, Jul 7, 2020 at 8:10 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> >> index f4a01305d9a6..a63a411a74d5 100644
> >> --- a/include/uapi/asm-generic/unistd.h
> >> +++ b/include/uapi/asm-generic/unistd.h
>
> OK, I'll rebase the next series on linux-next.
Just change the number to the next free one, without actually rebasing.
It's always a bit messy to have multiple syscalls added, but I think that
causes the least confusion.
> Do you know if there is other syscalls on their way to linux-next?
None that I'm aware of.
> Are the other parts of arch/syscall OK for you?
The arch/* and include/uapi/asm-generic changes look ok to me.
I'll reply to the syscall implementation separately/
Arnd
^ permalink raw reply
* Re: [PATCH v19 09/12] arch: Wire up landlock() syscall
From: Mickaël Salaün @ 2020-07-08 7:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Al Viro, Andy Lutomirski,
Anton Ivanov, Casey Schaufler, James Morris, Jann Horn, Jeff Dike,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Richard Weinberger, Serge E . Hallyn,
Shuah Khan, Vincent Dagonneau, Kernel Hardening, Linux API,
linux-arch, open list:DOCUMENTATION, Linux FS-devel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, LSM List,
the arch/x86 maintainers
In-Reply-To: <CAK8P3a0docCqHkEn9C7=e0GC_ieN1dsYgKQ9PbUmSZYxh9MRnw@mail.gmail.com>
On 08/07/2020 09:22, Arnd Bergmann wrote:
> On Tue, Jul 7, 2020 at 8:10 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>> index f4a01305d9a6..a63a411a74d5 100644
>> --- a/include/uapi/asm-generic/unistd.h
>> +++ b/include/uapi/asm-generic/unistd.h
>> @@ -857,9 +857,11 @@ __SYSCALL(__NR_openat2, sys_openat2)
>> __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
>> #define __NR_faccessat2 439
>> __SYSCALL(__NR_faccessat2, sys_faccessat2)
>> +#define __NR_landlock 440
>> +__SYSCALL(__NR_landlock, sys_landlock)
>>
>> #undef __NR_syscalls
>> -#define __NR_syscalls 440
>> +#define __NR_syscalls 441
>
> In linux-next, we already have:
>
> +#define __NR_watch_mount 440
> +#define __NR_watch_sb 441
> +#define __NR_fsinfo 442
> +#define __NR_process_madvise 443
>
> You may want to increase the number again.
>
> Arnd
>
OK, I'll rebase the next series on linux-next. Do you know if there is
other syscalls on their way to linux-next?
Are the other parts of arch/syscall OK for you?
^ permalink raw reply
* Re: [PATCH v19 09/12] arch: Wire up landlock() syscall
From: Arnd Bergmann @ 2020-07-08 7:22 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel@vger.kernel.org, Al Viro, Andy Lutomirski,
Anton Ivanov, Casey Schaufler, James Morris, Jann Horn, Jeff Dike,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Richard Weinberger, Serge E . Hallyn,
Shuah Khan, Vincent Dagonneau, Kernel Hardening, Linux API,
linux-arch, open list:DOCUMENTATION, Linux FS-devel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, LSM List,
the arch/x86 maintainers
In-Reply-To: <20200707180955.53024-10-mic@digikod.net>
On Tue, Jul 7, 2020 at 8:10 PM Mickaël Salaün <mic@digikod.net> wrote:
> index f4a01305d9a6..a63a411a74d5 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -857,9 +857,11 @@ __SYSCALL(__NR_openat2, sys_openat2)
> __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
> #define __NR_faccessat2 439
> __SYSCALL(__NR_faccessat2, sys_faccessat2)
> +#define __NR_landlock 440
> +__SYSCALL(__NR_landlock, sys_landlock)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 440
> +#define __NR_syscalls 441
In linux-next, we already have:
+#define __NR_watch_mount 440
+#define __NR_watch_sb 441
+#define __NR_fsinfo 442
+#define __NR_process_madvise 443
You may want to increase the number again.
Arnd
^ permalink raw reply
* Re: [PATCH v19 07/12] landlock: Support filesystem access-control
From: Mickaël Salaün @ 2020-07-08 7:03 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: Al Viro, Andy Lutomirski, Anton Ivanov, Arnd Bergmann,
Casey Schaufler, James Morris, Jann Horn, Jeff Dike,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Richard Weinberger, Serge E . Hallyn,
Shuah Khan, Vincent Dagonneau, kernel-hardening, linux-api,
linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <6a80b712-a7b9-7b47-083a-08b7769016f8@infradead.org>
On 07/07/2020 22:11, Randy Dunlap wrote:
> Hi--
>
> On 7/7/20 11:09 AM, Mickaël Salaün wrote:
>> ---
>> arch/Kconfig | 7 +
>> arch/um/Kconfig | 1 +
>> include/uapi/linux/landlock.h | 78 +++++
>> security/landlock/Kconfig | 2 +-
>> security/landlock/Makefile | 2 +-
>> security/landlock/fs.c | 609 ++++++++++++++++++++++++++++++++++
>> security/landlock/fs.h | 60 ++++
>> security/landlock/setup.c | 7 +
>> security/landlock/setup.h | 2 +
>> 9 files changed, 766 insertions(+), 2 deletions(-)
>> create mode 100644 include/uapi/linux/landlock.h
>> create mode 100644 security/landlock/fs.c
>> create mode 100644 security/landlock/fs.h
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 8cc35dc556c7..483b7476ac69 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -845,6 +845,13 @@ config COMPAT_32BIT_TIME
>> config ARCH_NO_PREEMPT
>> bool
>>
>> +config ARCH_EPHEMERAL_STATES
>> + def_bool n
>> + help
>> + An arch should select this symbol if it do not keep an internal kernel
>
> it does not
>
>> + state for kernel objects such as inodes, but instead rely on something
>
> instead relies on
>
>> + else (e.g. the host kernel for an UML kernel).
>> +
>> config ARCH_SUPPORTS_RT
>> bool
>>
>
> thanks.
>
Thanks Randy!
^ permalink raw reply
* Re: [PATCH v3 10/16] exec: Remove do_execve_file
From: Luis Chamberlain @ 2020-07-08 6:35 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Linus Torvalds, Christian Brauner,
Greg Kroah-Hartman
In-Reply-To: <20200702164140.4468-10-ebiederm@xmission.com>
On Thu, Jul 02, 2020 at 11:41:34AM -0500, Eric W. Biederman wrote:
> Now that the last callser has been removed remove this code from exec.
>
> For anyone thinking of resurrecing do_execve_file please note that
> the code was buggy in several fundamental ways.
>
> - It did not ensure the file it was passed was read-only and that
> deny_write_access had been called on it. Which subtlely breaks
> invaniants in exec.
>
> - The caller of do_execve_file was expected to hold and put a
> reference to the file, but an extra reference for use by exec was
> not taken so that when exec put it's reference to the file an
> underflow occured on the file reference count.
Maybe its my growing love with testing, but I'm going to have to partly
blame here that we added a new API without any respective testing.
Granted, I recall this this patch set could have used more wider review
and a bit more patience... but just mentioning this so we try to avoid
new api-without-testing with more reason in the future.
But more importantly, *how* could we have caught this? Or how can we
catch this sort of stuff better in the future?
> - The point of the interface was so that a pathname did not need to
> exist. Which breaks pathname based LSMs.
Perhaps so but this fails to do justice of the LSM consideration done
for the patch which added this during patch review [0], and I
particularly recall I called out LSM folks to bring their ray guns out at
this patch. It didn't get much attention.
Let me recap a few points I think your commit log should somehow
consider. You do as you please.
Users of shmem_kernel_file_setup() spawned out of the desire to
*avoid* LSMs since it didn't make sense in their case as their inodes
are never exposed to userspace. Such is the case for ipc/shm.c and
security/keys/big_key.c. Refer to commit c7277090927a5 ("security: shmem:
implement kernel private shmem inodes") and then commit e1832f2923ec9
("ipc: use private shmem or hugetlbfs inodes for shm segments").
And the umh module approach was doing:
a) mapping data already extracted by the kernel somehow from
a file somehow, presumably from /lib/modules/ path somewhere, but
again this is not visible to umc.c, as it just gets called with:
fork_usermode_blob(void *data, size_t len, struct umh_info *info)
b) Creating the respective tmpfs file with shmem_kernel_file_setup()
c) Populating the file created and stuffing it with our data passed
d) Calling do_execve_file() on it.
So, although I was hoping LSM folks would chime in for things I may have
missed during my patch review, my recollection from the patch thread was
that this becuase of a) it in theory could skip out on dealing with LSMs.
[0] https://lkml.kernel.org/r/20180509022526.hertzfpvy7apz6ny@ast-mbp
Luis
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Luis Chamberlain @ 2020-07-08 5:20 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Linus Torvalds
In-Reply-To: <87bll17ili.fsf_-_@x220.int.ebiederm.org>
On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
>
> I have tested thes changes by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
>
> I have compiled tested each change with and without CONFIG_BPFILTER
> enabled.
Sounds like grounds for a selftests driver and respective selftest?
And if so, has the other issues Tetsuo reported be hacked into one?
Luis
^ permalink raw reply
* Re: [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf
From: Scott Branden @ 2020-07-08 4:51 UTC (permalink / raw)
To: Florian Fainelli, Luis Chamberlain, Wolfram Sang,
Greg Kroah-Hartman, David Brown, Alexander Viro, Shuah Khan,
bjorn.andersson, Shuah Khan, Arnd Bergmann
Cc: Mimi Zohar, Rafael J . Wysocki, linux-kernel, linux-arm-msm,
linux-fsdevel, BCM Kernel Feedback, Olof Johansson, Andrew Morton,
Dan Carpenter, Colin Ian King, Kees Cook, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <c8bbabe6-0b25-a816-f95d-8af63010eaf2@gmail.com>
Hi Florian,
On 2020-07-07 9:38 p.m., Florian Fainelli wrote:
>
> On 7/6/2020 4:23 PM, Scott Branden wrote:
>> This patch series adds partial read support via a new call
>> request_partial_firmware_into_buf.
>> Such support is needed when the whole file is not needed and/or
>> only a smaller portion of the file will fit into allocated memory
>> at any one time.
>> In order to accept the enhanced API it has been requested that kernel
>> selftests and upstreamed driver utilize the API enhancement and so
>> are included in this patch series.
>>
>> Also in this patch series is the addition of a new Broadcom VK driver
>> utilizing the new request_firmware_into_buf enhanced API.
>>
>> Further comment followed to add IMA support of the partial reads
>> originating from request_firmware_into_buf calls. And another request
>> to move existing kernel_read_file* functions to its own include file.
> Do you have any way to separate the VK drivers submission from the
> request_partial_firmware_into_buf() work that you are doing? It looks
> like it is going to require quite a few iterations of this patch set for
> the firmware/fs/IMA part to be ironed out, so if you could get your
> driver separated out, it might help you achieve partial success here.
Originally I did not submit the driver.
But Greg K-H rejected the pread support unless there was an actual user
in the kernel.
Hence the need to submit this all in the patch series.
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-07-08 4:46 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Al Viro, Casey Schaufler, Alexei Starovoitov, linux-kernel,
David Miller, Greg Kroah-Hartman, Kees Cook, Andrew Morton,
Alexei Starovoitov, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
LSM List, Luis Chamberlain, Linus Torvalds
In-Reply-To: <ec6a6e18-d7aa-3072-c8dc-b925398b8409@i-love.sakura.ne.jp>
Just to make certain I understand what is going on I instrumented a
kernel with some print statements.
a) The workqueues and timers start before populate_rootfs.
b) populate_rootfs does indeed happen long before the bpfilter
module is intialized.
c) What prevents populate_rootfs and the umd_load_blob from
having problems when they call flush_delayed_put is the
fact that fput_many does:
"schedule_delayed_work(&delayed_fput_work,1)".
That 1 requests a delay of at least 1 jiffy. A jiffy is between
1ms and 10ms depending on how Linux is configured.
In my test configuration running a kernel in kvm printing to a serial
console I measured 0.8ms between the fput in blob_to_mnt and
flush_delayed_fput which immediately follows it.
So unless the fput becomes incredibly slow there is nothing to worry
about in blob_to_mnt.
d) As the same mechanism is used by populate_rootfs. A but in the
mechanism applies to both.
e) No one appears to have reported a problem executing files out of
initramfs these last several years since the flush_delayed_fput was
introduced.
f) The code works for me. There is real reason to believe the code will
work for everyone else, as the exact same logic is used by initramfs.
So it should be perfectly fine for the patchset and the
usermode_driver code to go ahead as written.
h) If there is something to be fixed it is flush_delayed_fput as that is
much more important than anything in the usermode driver code.
Eric
p.s.) When I talked of restarts of the usermode driver code ealier I was
referring to the code that restarts the usermode driver if it is
killed, the next time the kernel tries to talk to it.
That could mask an -ETXTBUSY except if it happens on the first exec
the net/bfilter/bpfilter_kern.c:load_umh() will return an error.
^ permalink raw reply
* Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support
From: Scott Branden @ 2020-07-08 4:41 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <42169718-d1b8-27f8-eeee-6cdef75a30d9@broadcom.com>
Hi Kees,
one more comment below.
On 2020-07-07 9:01 p.m., Scott Branden wrote:
>
>
> On 2020-07-07 4:56 p.m., Kees Cook wrote:
>> On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
>>> Add kernel_pread_file* support to kernel to allow for partial read
>>> of files with an offset into the file.
>>>
>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>> ---
>>> fs/exec.c | 93
>>> ++++++++++++++++++++++++--------
>>> include/linux/kernel_read_file.h | 17 ++++++
>>> 2 files changed, 87 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/fs/exec.c b/fs/exec.c
>>> index 4ea87db5e4d5..e6a8a65f7478 100644
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -928,10 +928,14 @@ struct file *open_exec(const char *name)
>>> }
>>> EXPORT_SYMBOL(open_exec);
>>> -int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>> - loff_t max_size, enum kernel_read_file_id id)
>>> -{
>>> - loff_t i_size, pos;
>>> +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
>>> + loff_t max_size, loff_t pos,
>>> + enum kernel_read_file_id id)
>>> +{
>>> + loff_t alloc_size;
>>> + loff_t buf_pos;
>>> + loff_t read_end;
>>> + loff_t i_size;
>>> ssize_t bytes = 0;
>>> int ret;
>>> @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void
>>> **buf, loff_t *size,
>>> ret = -EINVAL;
>>> goto out;
>>> }
>>> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
>>> +
>>> + /* Default read to end of file */
>>> + read_end = i_size;
>>> +
>>> + /* Allow reading partial portion of file */
>>> + if ((id == READING_FIRMWARE_PARTIAL_READ) &&
>>> + (i_size > (pos + max_size)))
>>> + read_end = pos + max_size;
>> There's no need to involve "id" here. There are other signals about
>> what's happening (i.e. pos != 0, max_size != i_size, etc).
> There are other signals other than the fact that kernel_read_file
> requires
> the entire file to be read while kernel_pread_file allows partial
> files to be read.
> So if you do a pread at pos = 0 you need another key to indicate it is
> "ok" if max_size < i_size.
> If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to
> share 99% of the code
> between kernel_read_file and kernel_pread_file then I need to add
> another parameter to a common function
> called between these functions. And adding another parameter was
> rejected previously in the review as a "swiss army knife approach" by
> another reviewer. I am happy to add it back in because it is
> necessary to share code and differentiate whether we are performing a
> partial read or not.
>>
>>> +
>>> + alloc_size = read_end - pos;
>>> + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size >
>>> max_size)) {
>>> ret = -EFBIG;
>>> goto out;
>>> }
>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>> - *buf = vmalloc(i_size);
>>> + if ((id != READING_FIRMWARE_PARTIAL_READ) &&
>>> + (id != READING_FIRMWARE_PREALLOC_BUFFER))
>>> + *buf = vmalloc(alloc_size);
>>> if (!*buf) {
>>> ret = -ENOMEM;
>>> goto out;
>>> }
>> The id usage here was a mistake in upstream, and the series I sent is
>> trying to clean that up.
> I see that cleanup and it works fine with the pread. Other than I
> need some sort of key to share code and indicate whether it is "ok" to
> do a partial read of the file or not.
>>
>> Greg, it seems this series is going to end up in your tree due to it
>> being drivers/misc? I guess I need to direct my series to Greg then, but
>> get LSM folks Acks.
>>
>>> - pos = 0;
>>> - while (pos < i_size) {
>>> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
>>> + buf_pos = 0;
>>> + while (pos < read_end) {
>>> + bytes = kernel_read(file, *buf + buf_pos, read_end - pos,
>>> &pos);
>>> if (bytes < 0) {
>>> ret = bytes;
>>> goto out_free;
>>> @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void
>>> **buf, loff_t *size,
>>> if (bytes == 0)
>>> break;
>>> +
>>> + buf_pos += bytes;
>>> }
>>> - if (pos != i_size) {
>>> + if (pos != read_end) {
>>> ret = -EIO;
>>> goto out_free;
>>> }
>>> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
>>> + ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
>>> if (!ret)
>>> *size = pos;
>> This call cannot be inside kernel_pread_file(): any future LSMs will see
>> a moving window of contents, etc. It'll need to be in kernel_read_file()
>> proper.
> If IMA still passes (after testing my next patch series with your
> changes and my modifications)
> I will need some more help here.
>>
>>> out_free:
>>> if (ret < 0) {
>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
>>> + if ((id != READING_FIRMWARE_PARTIAL_READ) &&
>>> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) {
>>> vfree(*buf);
>>> *buf = NULL;
>>> }
>>> @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void
>>> **buf, loff_t *size,
>>> allow_write_access(file);
>>> return ret;
>>> }
>>> +
>>> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>> + loff_t max_size, enum kernel_read_file_id id)
>>> +{
>>> + return kernel_pread_file(file, buf, size, max_size, 0, id);
>>> +}
>>> EXPORT_SYMBOL_GPL(kernel_read_file);
>>> -int kernel_read_file_from_path(const char *path, void **buf,
>>> loff_t *size,
>>> - loff_t max_size, enum kernel_read_file_id id)
>>> +int kernel_pread_file_from_path(const char *path, void **buf,
>>> + loff_t *size,
>>> + loff_t max_size, loff_t pos,
>>> + enum kernel_read_file_id id)
>>> {
>>> struct file *file;
>>> int ret;
>>> @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char
>>> *path, void **buf, loff_t *size,
>>> if (IS_ERR(file))
>>> return PTR_ERR(file);
>>> - ret = kernel_read_file(file, buf, size, max_size, id);
>>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id);
>>> fput(file);
>>> return ret;
>>> }
>>> +
>>> +int kernel_read_file_from_path(const char *path, void **buf, loff_t
>>> *size,
>>> + loff_t max_size, enum kernel_read_file_id id)
>>> +{
>>> + return kernel_pread_file_from_path(path, buf, size, max_size,
>>> 0, id);
>>> +}
>>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
>>> -int kernel_read_file_from_path_initns(const char *path, void **buf,
>>> - loff_t *size, loff_t max_size,
>>> - enum kernel_read_file_id id)
>>> +int kernel_pread_file_from_path_initns(const char *path, void **buf,
>>> + loff_t *size,
>>> + loff_t max_size, loff_t pos,
>>> + enum kernel_read_file_id id)
>>> {
>>> struct file *file;
>>> struct path root;
>>> @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const
>>> char *path, void **buf,
>>> if (IS_ERR(file))
>>> return PTR_ERR(file);
>>> - ret = kernel_read_file(file, buf, size, max_size, id);
>>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id);
>>> fput(file);
>>> return ret;
>>> }
>>> +
>>> +int kernel_read_file_from_path_initns(const char *path, void **buf,
>>> + loff_t *size, loff_t max_size,
>>> + enum kernel_read_file_id id)
>>> +{
>>> + return kernel_pread_file_from_path_initns(path, buf, size,
>>> max_size, 0, id);
>>> +}
>>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
>>> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size,
>>> loff_t max_size,
>>> - enum kernel_read_file_id id)
>>> +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
>>> + loff_t max_size, loff_t pos,
>>> + enum kernel_read_file_id id)
>>> {
>>> struct fd f = fdget(fd);
>>> int ret = -EBADF;
>>> @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void
>>> **buf, loff_t *size, loff_t max_size,
>>> if (!f.file)
>>> goto out;
>>> - ret = kernel_read_file(f.file, buf, size, max_size, id);
>>> + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id);
>>> out:
>>> fdput(f);
>>> return ret;
>>> }
>>> +
>>> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size,
>>> loff_t max_size,
>>> + enum kernel_read_file_id id)
>>> +{
>>> + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
>>> +}
>>> EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
>> For each of these execution path, the mapping to LSM hooks is:
>>
>> - all path must call security_kernel_read_file(file, id) before reading
>> (this appears to be fine as-is in your series).
>>
>> - anything doing a "full" read needs to call
>> security_kernel_post_read_file() with the file and full buffer, size,
>> etc (so all the kernel_read_file*() paths). I imagine instead of
>> adding 3 copy/pasted versions of this, it may be possible to refactor
>> the helpers into a single core "full" caller that takes struct file,
>> or doing some logic in kernel_pread_file() that notices it has the
>> entire file in the buffer and doing the call then.
>> As an example of what I mean about doing the call, here's how I might
>> imagine it for one of the paths if it took struct file:
>>
>> int kernel_read_file_from_file(struct file *file, void **buf, loff_t
>> *size,
>> loff_t max_size, enum kernel_read_file_id id)
>> {
>> int ret;
>>
>> ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
>> if (ret)
>> return ret;
>> return security_kernel_post_read_file(file, buf, *size, id);
>> }
>>
>>> #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
>>> diff --git a/include/linux/kernel_read_file.h
>>> b/include/linux/kernel_read_file.h
>>> index 53f5ca41519a..f061ccb8d0b4 100644
>>> --- a/include/linux/kernel_read_file.h
>>> +++ b/include/linux/kernel_read_file.h
>>> @@ -8,6 +8,7 @@
>>> #define __kernel_read_file_id(id) \
>>> id(UNKNOWN, unknown) \
>>> id(FIRMWARE, firmware) \
>>> + id(FIRMWARE_PARTIAL_READ, firmware) \
>>> id(FIRMWARE_PREALLOC_BUFFER, firmware) \
>>> id(FIRMWARE_EFI_EMBEDDED, firmware) \
>> And again, sorry that this was in here as a misleading example.
>>
>>> id(MODULE, kernel-module) \
>>> @@ -36,15 +37,31 @@ static inline const char
>>> *kernel_read_file_id_str(enum kernel_read_file_id id)
>>> return kernel_read_file_str[id];
>>> }
>>> +int kernel_pread_file(struct file *file,
>>> + void **buf, loff_t *size, loff_t pos,
>>> + loff_t max_size,
>>> + enum kernel_read_file_id id);
>>> int kernel_read_file(struct file *file,
>>> void **buf, loff_t *size, loff_t max_size,
>>> enum kernel_read_file_id id);
>>> +int kernel_pread_file_from_path(const char *path,
>>> + void **buf, loff_t *size, loff_t pos,
>>> + loff_t max_size,
>>> + enum kernel_read_file_id id);
>>> int kernel_read_file_from_path(const char *path,
>>> void **buf, loff_t *size, loff_t max_size,
>>> enum kernel_read_file_id id);
>>> +int kernel_pread_file_from_path_initns(const char *path,
>>> + void **buf, loff_t *size, loff_t pos,
>>> + loff_t max_size,
>>> + enum kernel_read_file_id id);
>>> int kernel_read_file_from_path_initns(const char *path,
>>> void **buf, loff_t *size, loff_t max_size,
>>> enum kernel_read_file_id id);
>>> +int kernel_pread_file_from_fd(int fd,
>>> + void **buf, loff_t *size, loff_t pos,
>>> + loff_t max_size,
>>> + enum kernel_read_file_id id);
>>> int kernel_read_file_from_fd(int fd,
>>> void **buf, loff_t *size, loff_t max_size,
>>> enum kernel_read_file_id id);
>> I remain concerned that adding these helpers will lead a poor
>> interaction with LSMs, but I guess I get to hold my tongue. :)
I only need kernel_pread_file and kernel_pread_file_from_path_initns.
kernel_pread_file_from_fd and kernel_pread_file_from_path were only
added for completeness.
And are really only helper functions called by their kernel_read_file*
counterparts at this time. So they can be removed from this patch if
that helps?
> We could add pread functions that are "unsafe" in nature instead then?
> As I certainly do not need any integrity checks on the file for my
> driver. The real check is done by the card the data is loaded to
> whether is passes the linux security checks or not.
> And then, if someone does want to do something "safe" with preads
> another kernel_read_file_securelock/unlock could be added for those
> that need security for their partial reads?
>>
>
^ permalink raw reply
* Re: [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf
From: Florian Fainelli @ 2020-07-08 4:38 UTC (permalink / raw)
To: Scott Branden, Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman,
David Brown, Alexander Viro, Shuah Khan, bjorn.andersson,
Shuah Khan, Arnd Bergmann
Cc: Mimi Zohar, Rafael J . Wysocki, linux-kernel, linux-arm-msm,
linux-fsdevel, BCM Kernel Feedback, Olof Johansson, Andrew Morton,
Dan Carpenter, Colin Ian King, Kees Cook, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <20200706232309.12010-1-scott.branden@broadcom.com>
On 7/6/2020 4:23 PM, Scott Branden wrote:
> This patch series adds partial read support via a new call
> request_partial_firmware_into_buf.
> Such support is needed when the whole file is not needed and/or
> only a smaller portion of the file will fit into allocated memory
> at any one time.
> In order to accept the enhanced API it has been requested that kernel
> selftests and upstreamed driver utilize the API enhancement and so
> are included in this patch series.
>
> Also in this patch series is the addition of a new Broadcom VK driver
> utilizing the new request_firmware_into_buf enhanced API.
>
> Further comment followed to add IMA support of the partial reads
> originating from request_firmware_into_buf calls. And another request
> to move existing kernel_read_file* functions to its own include file.
Do you have any way to separate the VK drivers submission from the
request_partial_firmware_into_buf() work that you are doing? It looks
like it is going to require quite a few iterations of this patch set for
the firmware/fs/IMA part to be ironed out, so if you could get your
driver separated out, it might help you achieve partial success here.
--
Florian
^ permalink raw reply
* Re: [PATCH v10 7/9] misc: bcm-vk: add Broadcom VK driver
From: Scott Branden @ 2020-07-08 4:30 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module, Desmond Yan, James Hu
In-Reply-To: <202007071700.C567BA7B@keescook>
On 2020-07-07 5:03 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:07PM -0700, Scott Branden wrote:
>> Add Broadcom VK driver offload engine.
>> This driver interfaces to the VK PCIe offload engine to perform
>> should offload functions as video transcoding on multiple streams
>> in parallel. VK device is booted from files loaded using
>> request_firmware_into_buf mechanism. After booted card status is updated
>> and messages can then be sent to the card.
>> Such messages contain scatter gather list of addresses
>> to pull data from the host to perform operations on.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
> nit: your S-o-b chain doesn't make sense (I would expect you at the end
> since you're sending it and showing as the Author). Is it Co-developed-by?
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Yes, Co-developed-by. Will adjust.
>
>> [...]
>> +
>> + max_buf = SZ_4M;
>> + bufp = dma_alloc_coherent(dev,
>> + max_buf,
>> + &boot_dma_addr, GFP_KERNEL);
>> + if (!bufp) {
>> + dev_err(dev, "Error allocating 0x%zx\n", max_buf);
>> + ret = -ENOMEM;
>> + goto err_buf_out;
>> + }
>> +
>> + bcm_vk_buf_notify(vk, bufp, boot_dma_addr, max_buf);
>> + } else {
>> + dev_err(dev, "Error invalid image type 0x%x\n", load_type);
>> + ret = -EINVAL;
>> + goto err_buf_out;
>> + }
>> +
>> + ret = request_partial_firmware_into_buf(&fw, filename, dev,
>> + bufp, max_buf, 0);
> Unless I don't understand what's happening here, this needs to be
> reordered if you're going to keep Mimi happy and disallow the device
> being able to see the firmware before it has been verified. (i.e. please
> load the firmware before mapping DMA across the buffer.)
I don't understand your concern here. We request partial firmware into
a buffer that we allocated.
After loading it we signal the card the firmware has been loaded into
that memory region.
The card then pulls the data into its internal memory. And,
authenticates it.
Even if the card randomly read and writes to that buffer it shouldn't
matter to the linux kernel security subsystem.
It passed the security check already when placed in the buffer.
If there is a concern could we add an "nosecurity"
request_partial_firmware_into_buf instead as there is no need for any
security on this particular request?
^ permalink raw reply
* Re: [PATCH v10 4/9] test_firmware: add partial read support for request_firmware_into_buf
From: Scott Branden @ 2020-07-08 4:09 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <202007071659.38721F7@keescook>
On 2020-07-07 4:59 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:04PM -0700, Scott Branden wrote:
>> Add additional hooks to test_firmware to pass in support
>> for partial file read using request_firmware_into_buf.
>> buf_size: size of buffer to request firmware into
>> partial: indicates that a partial file request is being made
>> file_offset: to indicate offset into file to request
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> I am a fan of tests. :) If Luis gives an Ack here, you're good.
There were not even any tests for request_firmware_into_buf before I
started this partial read support.
Fortunately those base changes have already been accepted so I think
this change is a simple addition to those accepted patches.
>
^ permalink raw reply
* Re: [PATCH v10 3/9] firmware: add request_partial_firmware_into_buf
From: Scott Branden @ 2020-07-08 4:07 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <202007071657.55C2CFA57@keescook>
On 2020-07-07 4:58 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:03PM -0700, Scott Branden wrote:
>> Add request_partial_firmware_into_buf to allow for portions
>> of firmware file to be read into a buffer. Necessary where firmware
>> needs to be loaded in portions from file in memory constrained systems.
> Just tear out the differing "id" and just use FW_OPT_PARTIAL and I think
> if Luis is happy, you're all set.
>
I hope so. Also, I will need to call
kernel_pread_file_from_path_initns() if FW_OPT_PARTIAL is set
and kernel_read_file_from_path_initns() otherwise to avoid a swiss
army-knife approach of calling a common function with multiple parameters.
^ permalink raw reply
* Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support
From: Scott Branden @ 2020-07-08 4:01 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <202007071642.AA705B2A@keescook>
On 2020-07-07 4:56 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
>> Add kernel_pread_file* support to kernel to allow for partial read
>> of files with an offset into the file.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>> fs/exec.c | 93 ++++++++++++++++++++++++--------
>> include/linux/kernel_read_file.h | 17 ++++++
>> 2 files changed, 87 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 4ea87db5e4d5..e6a8a65f7478 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -928,10 +928,14 @@ struct file *open_exec(const char *name)
>> }
>> EXPORT_SYMBOL(open_exec);
>>
>> -int kernel_read_file(struct file *file, void **buf, loff_t *size,
>> - loff_t max_size, enum kernel_read_file_id id)
>> -{
>> - loff_t i_size, pos;
>> +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
>> + loff_t max_size, loff_t pos,
>> + enum kernel_read_file_id id)
>> +{
>> + loff_t alloc_size;
>> + loff_t buf_pos;
>> + loff_t read_end;
>> + loff_t i_size;
>> ssize_t bytes = 0;
>> int ret;
>>
>> @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>> ret = -EINVAL;
>> goto out;
>> }
>> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
>> +
>> + /* Default read to end of file */
>> + read_end = i_size;
>> +
>> + /* Allow reading partial portion of file */
>> + if ((id == READING_FIRMWARE_PARTIAL_READ) &&
>> + (i_size > (pos + max_size)))
>> + read_end = pos + max_size;
> There's no need to involve "id" here. There are other signals about
> what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file requires
the entire file to be read while kernel_pread_file allows partial files
to be read.
So if you do a pread at pos = 0 you need another key to indicate it is
"ok" if max_size < i_size.
If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share
99% of the code
between kernel_read_file and kernel_pread_file then I need to add
another parameter to a common function
called between these functions. And adding another parameter was
rejected previously in the review as a "swiss army knife approach" by
another reviewer. I am happy to add it back in because it is necessary
to share code and differentiate whether we are performing a partial read
or not.
>
>> +
>> + alloc_size = read_end - pos;
>> + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
>> ret = -EFBIG;
>> goto out;
>> }
>>
>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>> - *buf = vmalloc(i_size);
>> + if ((id != READING_FIRMWARE_PARTIAL_READ) &&
>> + (id != READING_FIRMWARE_PREALLOC_BUFFER))
>> + *buf = vmalloc(alloc_size);
>> if (!*buf) {
>> ret = -ENOMEM;
>> goto out;
>> }
> The id usage here was a mistake in upstream, and the series I sent is
> trying to clean that up.
I see that cleanup and it works fine with the pread. Other than I need
some sort of key to share code and indicate whether it is "ok" to do a
partial read of the file or not.
>
> Greg, it seems this series is going to end up in your tree due to it
> being drivers/misc? I guess I need to direct my series to Greg then, but
> get LSM folks Acks.
>
>>
>> - pos = 0;
>> - while (pos < i_size) {
>> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
>> + buf_pos = 0;
>> + while (pos < read_end) {
>> + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos);
>> if (bytes < 0) {
>> ret = bytes;
>> goto out_free;
>> @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>
>> if (bytes == 0)
>> break;
>> +
>> + buf_pos += bytes;
>> }
>>
>> - if (pos != i_size) {
>> + if (pos != read_end) {
>> ret = -EIO;
>> goto out_free;
>> }
>>
>> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
>> + ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
>> if (!ret)
>> *size = pos;
> This call cannot be inside kernel_pread_file(): any future LSMs will see
> a moving window of contents, etc. It'll need to be in kernel_read_file()
> proper.
If IMA still passes (after testing my next patch series with your
changes and my modifications)
I will need some more help here.
>
>>
>> out_free:
>> if (ret < 0) {
>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
>> + if ((id != READING_FIRMWARE_PARTIAL_READ) &&
>> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) {
>> vfree(*buf);
>> *buf = NULL;
>> }
>> @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>> allow_write_access(file);
>> return ret;
>> }
>> +
>> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
>> + loff_t max_size, enum kernel_read_file_id id)
>> +{
>> + return kernel_pread_file(file, buf, size, max_size, 0, id);
>> +}
>> EXPORT_SYMBOL_GPL(kernel_read_file);
>>
>> -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>> - loff_t max_size, enum kernel_read_file_id id)
>> +int kernel_pread_file_from_path(const char *path, void **buf,
>> + loff_t *size,
>> + loff_t max_size, loff_t pos,
>> + enum kernel_read_file_id id)
>> {
>> struct file *file;
>> int ret;
>> @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>> if (IS_ERR(file))
>> return PTR_ERR(file);
>>
>> - ret = kernel_read_file(file, buf, size, max_size, id);
>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id);
>> fput(file);
>> return ret;
>> }
>> +
>> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>> + loff_t max_size, enum kernel_read_file_id id)
>> +{
>> + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id);
>> +}
>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
>>
>> -int kernel_read_file_from_path_initns(const char *path, void **buf,
>> - loff_t *size, loff_t max_size,
>> - enum kernel_read_file_id id)
>> +int kernel_pread_file_from_path_initns(const char *path, void **buf,
>> + loff_t *size,
>> + loff_t max_size, loff_t pos,
>> + enum kernel_read_file_id id)
>> {
>> struct file *file;
>> struct path root;
>> @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf,
>> if (IS_ERR(file))
>> return PTR_ERR(file);
>>
>> - ret = kernel_read_file(file, buf, size, max_size, id);
>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id);
>> fput(file);
>> return ret;
>> }
>> +
>> +int kernel_read_file_from_path_initns(const char *path, void **buf,
>> + loff_t *size, loff_t max_size,
>> + enum kernel_read_file_id id)
>> +{
>> + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id);
>> +}
>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
>>
>> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
>> - enum kernel_read_file_id id)
>> +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
>> + loff_t max_size, loff_t pos,
>> + enum kernel_read_file_id id)
>> {
>> struct fd f = fdget(fd);
>> int ret = -EBADF;
>> @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
>> if (!f.file)
>> goto out;
>>
>> - ret = kernel_read_file(f.file, buf, size, max_size, id);
>> + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id);
>> out:
>> fdput(f);
>> return ret;
>> }
>> +
>> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
>> + enum kernel_read_file_id id)
>> +{
>> + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
>> +}
>> EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
> For each of these execution path, the mapping to LSM hooks is:
>
> - all path must call security_kernel_read_file(file, id) before reading
> (this appears to be fine as-is in your series).
>
> - anything doing a "full" read needs to call
> security_kernel_post_read_file() with the file and full buffer, size,
> etc (so all the kernel_read_file*() paths). I imagine instead of
> adding 3 copy/pasted versions of this, it may be possible to refactor
> the helpers into a single core "full" caller that takes struct file,
> or doing some logic in kernel_pread_file() that notices it has the
> entire file in the buffer and doing the call then.
> As an example of what I mean about doing the call, here's how I might
> imagine it for one of the paths if it took struct file:
>
> int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size,
> loff_t max_size, enum kernel_read_file_id id)
> {
> int ret;
>
> ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
> if (ret)
> return ret;
> return security_kernel_post_read_file(file, buf, *size, id);
> }
>
>>
>> #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
>> diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
>> index 53f5ca41519a..f061ccb8d0b4 100644
>> --- a/include/linux/kernel_read_file.h
>> +++ b/include/linux/kernel_read_file.h
>> @@ -8,6 +8,7 @@
>> #define __kernel_read_file_id(id) \
>> id(UNKNOWN, unknown) \
>> id(FIRMWARE, firmware) \
>> + id(FIRMWARE_PARTIAL_READ, firmware) \
>> id(FIRMWARE_PREALLOC_BUFFER, firmware) \
>> id(FIRMWARE_EFI_EMBEDDED, firmware) \
> And again, sorry that this was in here as a misleading example.
>
>> id(MODULE, kernel-module) \
>> @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
>> return kernel_read_file_str[id];
>> }
>>
>> +int kernel_pread_file(struct file *file,
>> + void **buf, loff_t *size, loff_t pos,
>> + loff_t max_size,
>> + enum kernel_read_file_id id);
>> int kernel_read_file(struct file *file,
>> void **buf, loff_t *size, loff_t max_size,
>> enum kernel_read_file_id id);
>> +int kernel_pread_file_from_path(const char *path,
>> + void **buf, loff_t *size, loff_t pos,
>> + loff_t max_size,
>> + enum kernel_read_file_id id);
>> int kernel_read_file_from_path(const char *path,
>> void **buf, loff_t *size, loff_t max_size,
>> enum kernel_read_file_id id);
>> +int kernel_pread_file_from_path_initns(const char *path,
>> + void **buf, loff_t *size, loff_t pos,
>> + loff_t max_size,
>> + enum kernel_read_file_id id);
>> int kernel_read_file_from_path_initns(const char *path,
>> void **buf, loff_t *size, loff_t max_size,
>> enum kernel_read_file_id id);
>> +int kernel_pread_file_from_fd(int fd,
>> + void **buf, loff_t *size, loff_t pos,
>> + loff_t max_size,
>> + enum kernel_read_file_id id);
>> int kernel_read_file_from_fd(int fd,
>> void **buf, loff_t *size, loff_t max_size,
>> enum kernel_read_file_id id);
> I remain concerned that adding these helpers will lead a poor
> interaction with LSMs, but I guess I get to hold my tongue. :)
We could add pread functions that are "unsafe" in nature instead then?
As I certainly do not need any integrity checks on the file for my
driver. The real check is done by the card the data is loaded to
whether is passes the linux security checks or not.
And then, if someone does want to do something "safe" with preads
another kernel_read_file_securelock/unlock could be added for those that
need security for their partial reads?
>
^ permalink raw reply
* Re: [PATCH v3 13/16] exit: Factor thread_group_exited out of pidfd_poll
From: Eric W. Biederman @ 2020-07-08 3:50 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Christian Brauner, Alexei Starovoitov, linux-kernel, David Miller,
Greg Kroah-Hartman, Tetsuo Handa, Kees Cook, Andrew Morton,
Alexei Starovoitov, Al Viro, bpf, linux-fsdevel, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds
In-Reply-To: <a84ec1df-dc9b-dd5b-cc34-385fd3ca1da4@iogearbox.net>
Daniel Borkmann <daniel@iogearbox.net> writes:
> Hey Eric, are you planning to push the final version into a topic branch
> so it can be pulled into bpf-next as discussed earlier?
Yes. I just about have it ready. I am taking one last pass through the
review comments to make certain I have not missed anything before I do.
I am hoping I can get it out tonight. Fingers crossed.
Eric
^ permalink raw reply
* Re: [PATCH v10 1/9] fs: move kernel_read_file* to its own include file
From: Scott Branden @ 2020-07-08 3:39 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <202007071637.ABF914AB@keescook>
On 2020-07-07 4:40 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:01PM -0700, Scott Branden wrote:
>> Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
>> include file. That header gets pulled in just about everywhere
>> and doesn't really need functions not related to the general fs interface.
>>
>> Suggested-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>> drivers/base/firmware_loader/main.c | 1 +
>> fs/exec.c | 1 +
>> include/linux/fs.h | 39 ----------------------
>> include/linux/ima.h | 1 +
>> include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++
>> include/linux/security.h | 1 +
>> kernel/kexec_file.c | 1 +
>> kernel/module.c | 1 +
>> security/integrity/digsig.c | 1 +
>> security/integrity/ima/ima_fs.c | 1 +
>> security/integrity/ima/ima_main.c | 1 +
>> security/integrity/ima/ima_policy.c | 1 +
>> security/loadpin/loadpin.c | 1 +
>> security/security.c | 1 +
>> security/selinux/hooks.c | 1 +
>> 15 files changed, 65 insertions(+), 39 deletions(-)
>> create mode 100644 include/linux/kernel_read_file.h
> This looks like too many files are getting touched. If it got added to
> security.h, very few of the above .c files will need it explicitly
> added (maybe none).
Some people want the header file added to each file that uses it,
others want it in a common header file. I tried to add it to each file
that uses it.
But if the other approach is to be followed that could be done.
> You can test future versions of this change with an
> allmodconfig build and make sure you have a matching .o for each .c
> file that calls kernel_read_file(). :)
>
> But otherwise, sure, seems good.
>
^ permalink raw reply
* Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support
From: Scott Branden @ 2020-07-08 3:35 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Wolfram Sang, Greg Kroah-Hartman, David Brown,
Alexander Viro, Shuah Khan, bjorn.andersson, Shuah Khan,
Arnd Bergmann, Mimi Zohar, Rafael J . Wysocki, linux-kernel,
linux-arm-msm, linux-fsdevel, BCM Kernel Feedback, Olof Johansson,
Andrew Morton, Dan Carpenter, Colin Ian King, Takashi Iwai,
linux-kselftest, Andy Gross, linux-integrity,
linux-security-module
In-Reply-To: <202007071456.50FD0054E@keescook>
On 2020-07-07 4:36 p.m., Kees Cook wrote:
> On Tue, Jul 07, 2020 at 10:13:42AM -0700, Scott Branden wrote:
>> You and others are certainly more experts in the filesystem and security
>> infrastructure of the kernel.
>> What I am trying to accomplish is a simple operation:
>> request part of a file into a buffer rather than the whole file.
>> If someone could add such support I would be more than happy to use it.
> Sure, and I totally understand that, but as it happens, no one has stepped
> up with spare time to do that work. Since you're the person with the need
> for the API, it falls to you to do it. And I understand what feature creep
> feels like (I needed to fix one design problem[1] with timers, and I spent
> months sending hundreds of patches). Some times you get lucky and it's
> easy, and sometimes you end up touching something that needs a LOT of work
> to refactor before you can make your desired change work well with the
> rest of the kernel and be maintainable by other people into the future.
>
> Quick tangent: I can't find in the many many threads where you explain
> how large these firmware images are and why existing kernel memory
> allocations are insufficient to load them. How large are these[2] files?
>
> /lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin
This is on the order of a few MB at most.
> /lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin
Some of these images are current 250MB. At we anticipate them growing
to 512MB.
And, we do have systems with the driver loading 16 cards in parallel
with no requirement that they are the same images
(although loading 16 different images to 16 different cards would be
strange but possible).
>
> For me, the requirements for partial read support are these things,
> which are the characteristics of the existing API:
>
> - the LSM must be able to validate the entire file contents before
> any data is available to any reader. (Which was pointed out back in
> August 2019[3].)
I thought this was addressed in patch v6 "ima: aad FIRMWARE_PARTIAL_READ
support"
https://lkml.org/lkml/2020/6/5/1126
(although implementation not to your liking in current review)?
> And "any" reader includes having a DMA window open
> on the memory range used for reading the contents (which was pointed
> out at by Mimi[4] but went unanswered and remains broken still in this
> v10, but I will comment separately on that.)
>
> - the integrity of the file contents must be maintained between
> validation and delivery (currently this is handled internally via
> disallow_writes()).
I don't understand what you are staying here: I am request a partial
firmware read into a buf.
At the time the partial firmware is read into a buf it is validated by
the security module if such integrity checks are enabled.
If, at another time I wish to request another partial firmware into a
buffer (could be the same part of the file or a different part of a file
or from another file), the integrity check is performed again and the
portion of the file I request should be put into the buffer.
If a lock on a file is needed by somebody between these partial reads
that is a different API and out of the scope of my patch series.
>> This has now bubbled into many other designs issues in the existing
>> codebase.
> Correct -- this is one of the many difficulties of contributing to a
> large and complex code base with many maintainers. There can be a lot
> of requirements for the code that have nothing to do with seemingly more
> narrow areas of endeavour.
Thanks at least for helping with guidance, I see your review is thought
out and
hopefully we can come to a conclusion as I feel we are fairly close with
your changes.
>
>> I will need more details on your comments - see below.
>>
>> On 2020-07-06 8:08 p.m., Kees Cook wrote:
>>> On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
>>>> Add FIRMWARE_PARTIAL_READ support for integrity
>>>> measurement on partial reads of firmware files.
>>> Hi,
>>>
>>> Several versions ago I'd suggested that the LSM infrastructure handle
>>> the "full read" semantics so that individual LSMs don't need to each
>>> duplicate the same efforts. As it happens, only IMA is impacted (SELinux
>>> ignores everything except modules, and LoadPin only cares about origin
>>> not contents).
>> Does your patch series "Fix misused kernel_read_file() enums" handle this
>> because this suggestion is outside the scope of my change?
> My proposed patch series cleans up a number of mistakes that were made
> to the kernel_read_file() API, and helps clarify my point about the
> enums being used for *what*, and not *how* or *where*, which needs to
> be fixed in this series and shouldn't be a big deal (I will reply to
> individual patches).
>
>>> Next is the problem that enum kernel_read_file_id is an object
>>> TYPE enum, not a HOW enum. (And it seems I missed the addition of
>>> READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
>>> That it's a partial read doesn't change _what_ you're reading: that's an
>>> internal API detail. What happens when I attempt to do a partial read of
>>> a kexec image?
>> It does not appear there is any user of partial reads of kexec images?
>> I have been informed by Greg K-H to not add apis that are not used so such
>> support doesn't make sense to add at this time.
> But you are proposing a generic API enhancement that any other user in
> the kernel may end up using. Just because the bcm-vk driver is the only
> user now, and IMA is the only LSM performing content analysis, it
> doesn't mean that there won't be another driver added later, nor another
> LSM. In fact, the new BPF LSM means that anything exposed by LSM hooks
> is now available for analysis.
>
>>> I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
>>> but the LSMs will have no idea it's a partial read.
>> The addition I am adding is for request_partial_firmware_into_buf.
>> In order to do so it adds internal support for partial reads of firmware
>> files,
>> not kexec image.
> Yes, but you're changing kernel_read_file() APIs to do it. There are
> plenty of users of that API. Maybe they would like to also use partial
> reads?
>
> $ git grep kernel_read_file | wc -l
> 77
>
>> The above seems outside the scope of my patch?
> Unfortunately, it is not. Part of your responsibility as a kernel
> developer making API changes/additions is that those changes need to
> interact correctly with the rest of the kernel (and be maintainable).
>
>>> Finally, what keeps the contents of the file from changing between the
>>> first call (which IMA will read the entire file for) and the next reads
>>> which will bypass IMA?
>> The request is for a partial read. IMA ensures the whole file integrity
>> even though I only do a partial read.
>> The next partial read will re-read and check integrity of file.
> So, while terribly inefficient, I guess this approach is tenable. It
> means that each partial read will trigger a full read for LSMs that care
> about the hook.
> So, to that end, I wonder why IMA doesn't do this for all file types?
>
> It also means that we won't have a strict pairing of
> security_kernel_read_file() to security_kernel_post_read_file() in the
> LSMs, but it seems that only IMA currently explicitly cares about this
> (or maybe not at all).
>
> I'm not entirely happy about using this design, but it does appear
> sufficient.
Yes, terribly inefficient, but if somebody wants to do some optimization
they are welcome to it.
In fact, I want to call a partial read of a file with NO security. Can I
add such a call instead?
If we did that now then the inefficient read of the file multiple times
to authenticate it each time wouldn't be introduced.
In my use case the linux kernel security on the file is quite
meaningless and a waste of time to even perform.
Whether the file has been compromised, corrupt or otherwise the image is
validated by the hardware after
the image is loaded to the card to ensure it passes authentication.
>
>>> I'd suggested that the open file must have writes
>>> disabled on it (as execve() does).
>> The file will be reopened and integrity checked on the next partial read (if
>> there is one).
>> So I don't think there is any change to be made here.
>> If writes aren't already disabled for a whole file read then that is
>> something that needs to be fixed in the existing code.
> My suggestion quoted here was operating on the idea that whole-file
> verification wasn't happening on every partial read, so this isn't a
> problem in that case.
>
>>> So, please redesign this:
>>> - do not add an enum
>> I used existing infrastructure provided by Mimi but now looks like it will
>> have to fit with your patches from yesterday.
> Right, this won't be hard. I will send a v2 of my patches to clarify the
> purpose of the 3 file content hooks (load_data, read_file,
> post_read_file), which might need renaming...
I see your cleanup and merged with it. Will need to test everything again.
>
>>> - make the file unwritable for the life of having the handle open
>> It's no different than a full file read so no change to be made here.
> Correct.
>
>>> - make the "full read" happen as part of the first partial read so the
>>> LSMs don't have to reimplement everything
>> Each partial read is an individual operation so I think a "full read" is
>> performed every time
>> if your security IMA is enabled. If someone wants to add a file lock and
>> then partial reads in the kernel
>> then that would be different than what is needed by the kernel driver.
> So, given that Mimi is (I think?) satisfied with your approach here, I
> can't realistically complain. I still don't like the idea of each LSM
> needing to perform it's full read loop for the contents, but so be it:
> IMA will have the code, SELinux doesn't care (yet), and LoadPin doesn't
> care about contents.
I obviously am not aware of all of the security hooks and architecture
in place but I do see your cleanups as a simplification over what was there.
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20170808003345.GA107289@beast/
> git log --oneline --grep 'Convert timer' --author "Kees Cook" | wc -l
> 271
> [2] https://lore.kernel.org/lkml/824407ae-8ab8-0fe3-bd72-270fce960ac5@broadcom.com/
> [3] https://lore.kernel.org/lkml/s5hsgpsqd49.wl-tiwai@suse.de/
> [4] https://lore.kernel.org/lkml/1591622862.4638.74.camel@linux.ibm.com/
>
^ permalink raw reply
* Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
From: Kees Cook @ 2020-07-08 3:14 UTC (permalink / raw)
To: Scott Branden
Cc: James Morris, Luis Chamberlain, Mimi Zohar, Greg Kroah-Hartman,
Rafael J. Wysocki, Alexander Viro, Jessica Yu, Dmitry Kasatkin,
Serge E. Hallyn, Casey Schaufler, Eric W. Biederman,
Peter Zijlstra, Matthew Garrett, David Howells,
Mauro Carvalho Chehab, Randy Dunlap, Joel Fernandes (Google),
KP Singh, Dave Olsthoorn, Hans de Goede, Peter Jones,
Andrew Morton, Stephen Boyd, Paul Moore, linux-kernel,
linux-fsdevel, linux-integrity, linux-security-module,
Christoph Hellwig
In-Reply-To: <c2e4f5ae-0a2f-454e-6847-c413ca719abf@broadcom.com>
On Tue, Jul 07, 2020 at 08:06:23PM -0700, Scott Branden wrote:
> Some people want the header files included in each c file they are used.
> Others want header files not included if they are included in another header
> file.
Ah, yes. That's fine then, leave them in the .c files.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 4/4] module: Add hook for security_kernel_post_read_file()
From: Kees Cook @ 2020-07-08 3:10 UTC (permalink / raw)
To: Mimi Zohar
Cc: James Morris, Jessica Yu, Luis Chamberlain, Scott Branden,
Greg Kroah-Hartman, Rafael J. Wysocki, Alexander Viro,
Dmitry Kasatkin, Serge E. Hallyn, Casey Schaufler,
Eric W. Biederman, Peter Zijlstra, Matthew Garrett, David Howells,
Mauro Carvalho Chehab, Randy Dunlap, Joel Fernandes (Google),
KP Singh, Dave Olsthoorn, Hans de Goede, Peter Jones,
Andrew Morton, Stephen Boyd, Paul Moore, linux-kernel,
linux-fsdevel, linux-integrity, linux-security-module
In-Reply-To: <1594169240.23056.143.camel@linux.ibm.com>
On Tue, Jul 07, 2020 at 08:47:20PM -0400, Mimi Zohar wrote:
> On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > Calls to security_kernel_load_data() should be paired with a call to
> > security_kernel_post_read_file() with a NULL file argument. Add the
> > missing call so the module contents are visible to the LSMs interested
> > in measuring the module content. (This also paves the way for moving
> > module signature checking out of the module core and into an LSM.)
> >
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > kernel/module.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 0c6573b98c36..af9679f8e5c6 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> > return -EFAULT;
> > }
> >
> > - return 0;
> > + err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> > + info->len, READING_MODULE);
>
> There was a lot of push back on calling security_kernel_read_file()
> with a NULL file descriptor here.[1] The result was defining a new
> security hook - security_kernel_load_data - and enumeration -
> LOADING_MODULE. I would prefer calling the same pre and post security
> hook.
>
> Mimi
>
> [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/007110.html
Ah yes, thanks for the pointer to the discussion.
I think we have four cases then, for differing LSM hooks:
- no "file", no contents
e.g. init_module() before copying user buffer
security_kernel_load_data()
- only a "file" available, no contents
e.g. kernel_read_file() before actually reading anything
security_kernel_read_file()
- "file" and contents
e.g. kernel_read_file() after reading
security_kernel_post_read_file()
- no "file" available, just the contents
e.g. firmware platform fallback from EFI space (no "file")
unimplemented!
If an LSM wants to be able to examine the contents of firmware, modules,
kexec, etc, it needs either a "file" or the full contents.
The "file" methods all pass through the kernel_read_file()-family. The
others happen via blobs coming from userspace or (more recently) the EFI
universe.
So, if a NULL file is unreasonable, we need, perhaps,
security_kernel_post_load_data()
?
--
Kees Cook
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox