| Message ID | 20240905152512.3781098-2-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Integrate with Integrity Digest Cache | expand |
On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Introduce a new hook to check the integrity of digest lists. "Introduce DIGEST_LIST_CHECK, a new hook..." > > The new hook is invoked during a kernel read with file type "with the file type" > READING_DIGEST LIST, which is done by the Integrity Digest Cache when it is > populating a digest cache with a digest list. The patch creates a new struct imap_rule_entry instance when it parses the corresponding rule, which means that there are couple slices of information missing here: 1. The commit message does not tell what the code change effectively is. I scavenged this information from [1]. 2. The commit message does no effort to connect the dots between the effective change and the expected goal. I'd put a lot of effort to this commit message assuming that the new hook is at the center of the goals of this patch set. [1] https://elixir.bootlin.com/linux/v6.10-rc4/source/security/integrity/ima/ima_policy.c#L1404 BR, Jarkko
On Fri, 2024-09-06 at 12:41 +0300, Jarkko Sakkinen wrote: > On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Introduce a new hook to check the integrity of digest lists. > > "Introduce DIGEST_LIST_CHECK, a new hook..." > > > > > The new hook is invoked during a kernel read with file type > > "with the file type" > > > > READING_DIGEST LIST, which is done by the Integrity Digest Cache when it is > > populating a digest cache with a digest list. > > The patch creates a new struct imap_rule_entry instance when it parses > the corresponding rule, which means that there are couple slices of > information missing here: > > 1. The commit message does not tell what the code change effectively > is. I scavenged this information from [1]. Sorry, to me it seems a bit redundant to state what a IMA hook is. The new hook will be handled by IMA like the other existing hooks. > 2. The commit message does no effort to connect the dots between the > effective change and the expected goal. Sure, will mention the goal better. Thanks Roberto > I'd put a lot of effort to this commit message assuming that the new > hook is at the center of the goals of this patch set. > > [1] https://elixir.bootlin.com/linux/v6.10-rc4/source/security/integrity/ima/ima_policy.c#L1404 > > BR, Jarkko
On Fri Sep 6, 2024 at 2:22 PM EEST, Roberto Sassu wrote: > On Fri, 2024-09-06 at 12:41 +0300, Jarkko Sakkinen wrote: > > On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > Introduce a new hook to check the integrity of digest lists. > > > > "Introduce DIGEST_LIST_CHECK, a new hook..." > > > > > > > > The new hook is invoked during a kernel read with file type > > > > "with the file type" > > > > > > > READING_DIGEST LIST, which is done by the Integrity Digest Cache when it is > > > populating a digest cache with a digest list. > > > > The patch creates a new struct imap_rule_entry instance when it parses > > the corresponding rule, which means that there are couple slices of > > information missing here: > > > > 1. The commit message does not tell what the code change effectively > > is. I scavenged this information from [1]. > > Sorry, to me it seems a bit redundant to state what a IMA hook is. The > new hook will be handled by IMA like the other existing hooks. I think with documentation (scoping also to commit messages) it is in general a good strategy to put it less rather than more. No documentation is better than polluted documentation ;-) Just remarking what might not be obvious with someone who might not be obvious, unless being a pro-active contributor. BR, Jarkko
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index c2385183826c..22237fec5532 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -36,6 +36,7 @@ Description: [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] [SETXATTR_CHECK][MMAP_CHECK_REQPROT] + [DIGEST_LIST_CHECK] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index ad5c95cf22ac..9d41d6b1cce2 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -317,6 +317,7 @@ static inline unsigned int ima_hash_key(u8 *digest) hook(KEY_CHECK, key) \ hook(CRITICAL_DATA, critical_data) \ hook(SETXATTR_CHECK, setxattr_check) \ + hook(DIGEST_LIST_CHECK, digest_list_check) \ hook(MAX_CHECK, none) #define __ima_hook_enumify(ENUM, str) ENUM, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 646d900828e0..cff8b5a12512 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -798,7 +798,8 @@ const int read_idmap[READING_MAX_ID] = { [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK, - [READING_POLICY] = POLICY_CHECK + [READING_POLICY] = POLICY_CHECK, + [READING_DIGEST_LIST] = DIGEST_LIST_CHECK, }; /** diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 09da8e639239..047d50c2eb57 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1290,6 +1290,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) case MODULE_CHECK: case KEXEC_KERNEL_CHECK: case KEXEC_INITRAMFS_CHECK: + case DIGEST_LIST_CHECK: if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC | IMA_UID | IMA_FOWNER | IMA_FSUUID | IMA_INMASK | IMA_EUID | IMA_PCR | @@ -1533,6 +1534,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->func = CRITICAL_DATA; else if (strcmp(args[0].from, "SETXATTR_CHECK") == 0) entry->func = SETXATTR_CHECK; + else if (strcmp(args[0].from, "DIGEST_LIST_CHECK") == 0) + entry->func = DIGEST_LIST_CHECK; else result = -EINVAL; if (!result)