Re: [RFC PATCH v7 07/16] ipe: add auditing support

From: Deven Bowers
Date: Tue Oct 26 2021 - 15:03:50 EST



On 10/15/2021 12:50 PM, Randy Dunlap wrote:
On 10/15/21 12:25 PM, Deven Bowers wrote:
On 10/13/2021 3:54 PM, Randy Dunlap wrote:
Hi,

On 10/13/21 12:06 PM, deven.desai@xxxxxxxxxxxxxxxxxxx wrote:
diff --git a/security/ipe/Kconfig b/security/ipe/Kconfig
index c4503083e92d..ef556b66e674 100644
--- a/security/ipe/Kconfig
+++ b/security/ipe/Kconfig
@@ -17,3 +17,55 @@ menuconfig SECURITY_IPE
        requirements on the fly.
          If unsure, answer N.
+
+if SECURITY_IPE
+
+choice
+    prompt "Hash algorithm used in auditing policies"
+    default IPE_AUDIT_HASH_SHA1
+    depends on AUDIT
+    help
+        Specify the hash algorithm used when auditing policies.
+        The hash is used to uniquely identify a policy from other
+        policies on the system.
+
+        If unsure, leave default.
+
+    config IPE_AUDIT_HASH_SHA1
+        bool "sha1"
+        depends on CRYPTO_SHA1
+        help
+            Use the SHA128 algorithm to hash policies
+            in the audit records.
+
+    config IPE_AUDIT_HASH_SHA256
+        bool "sha256"
+        depends on CRYPTO_SHA256
+        help
+            Use the SHA256 algorithm to hash policies
+            in the audit records.
+
+    config IPE_AUDIT_HASH_SHA384
+        bool "sha384"
+        depends on CRYPTO_SHA512
+        help
+            Use the SHA384 algorithm to hash policies
+            in the audit records
+
+    config IPE_AUDIT_HASH_SHA512
+        bool "sha512"
+        depends on CRYPTO_SHA512
+        help
+            Use the SHA512 algorithm to hash policies
+            in the audit records
+endchoice
+
+config IPE_AUDIT_HASH_ALG
+    string
+    depends on AUDIT
+    default "sha1" if IPE_AUDIT_HASH_SHA1
+    default "sha256" if IPE_AUDIT_HASH_SHA256
+    default "sha384" if IPE_AUDIT_HASH_SHA384
+    default "sha512" if IPE_AUDIT_HASH_SHA512
+
+endif

Please follow coding-style for Kconfig files:

(from Documentation/process/coding-style.rst, section 10):

For all of the Kconfig* configuration files throughout the source tree,
the indentation is somewhat different.  Lines under a ``config`` definition
are indented with one tab, while help text is indented an additional two
spaces.

Oof. That's embarrassing. Sorry, I'll fix this for v8.

While I'm at it, is the help text required for choice configs?
checkpatch --strict complains with a warning without them, but
I see other places in the tree where help text is omitted for
these configs attached to a choice.

Does checkpatch complain about what you have above
or did you add that help text to keep it from complaining?

I added the help text to keep it from complaining (and added it incorrectly,
clearly).



Documentation/process/* doesn't seem to have any guidance, nor
Documentation/kbuild/* on whether it is safe to ignore that
checkpatch warning.

Yeah, I don't think that we have any good guidance on that.

I would say that if the choice prompt provides good/adequate
help info, then each 'config' inside the choice block does not
need help text. OTOH, if the choice prompt has little/no help
info, then each 'config' under it should have some useful info.

I only looked in arch/x86/Kconfig, init/Kconfig, and lib/Kconfig.debug,
but you can see either help text method being used in those.

And then if the help text is adequate in either one of those
methods, I would just ignore the checkpatch complaints.
It's just a guidance tool.

Alright. I think the choice guidance is pretty clear:

Specify the hash algorithm used when auditing policies.
The hash is used to uniquely identify a policy from other
policies on the system.

So I'll remove the help text for these choices.

At worst, I can make the choice text more clear.

HTH.