* [PATCH v2] ipe: add errno field to IPE policy load auditing [not found] <CAKtyLkFg2+8ciy4DM=g+vcTVvuRPNL2SHbN+m9ObErxtYXZYPw@mail.gmail.com> @ 2025-02-27 22:46 ` Jasjiv Singh 2025-02-27 23:41 ` Fan Wu 0 siblings, 1 reply; 9+ messages in thread From: Jasjiv Singh @ 2025-02-27 22:46 UTC (permalink / raw) To: wufan Cc: audit, corbet, eparis, jasjivsingh, jmorris, linux-audit, linux-doc, linux-kernel, linux-security-module, paul Thanks for reviewing it. Here's the example generated from real logs: AUDIT_IPE_POLICY_LOAD(1422): audit: AUDIT1422 policy_name="Test_Policy" policy_version=0.0.1 policy_digest =sha256:84EFBA8FA71E62AE0A537FAB962F8A2BD1053964C4 299DCA92BFFF4DB82E86D3 auid=1000 ses=3 lsm=ipe res=1 errno=0 The above record shows a new policy has been successfully loaded into the kernel with the policy name, version, and hash with the errno=0. AUDIT_IPE_POLICY_LOAD(1422) with error: audit: AUDIT1422 policy_name=? policy_version=? policy_digest=? auid=1000 ses=3 lsm=ipe res=0 errno=-74 The above record shows a policy load failure due to an invalid policy. I have updated the failure cases in new_policy() and update_policy(), which covers each case as well. Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> --- Documentation/admin-guide/LSM/ipe.rst | 2 ++ security/ipe/audit.c | 10 ++++------ security/ipe/fs.c | 17 ++++++++++++----- security/ipe/policy.c | 4 +--- security/ipe/policy_fs.c | 24 +++++++++++++++++++----- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst index 2143165f48c9..5dbf54471fab 100644 --- a/Documentation/admin-guide/LSM/ipe.rst +++ b/Documentation/admin-guide/LSM/ipe.rst @@ -453,6 +453,8 @@ Field descriptions: | errno | integer | No | The result of the policy error as follows: | | | | | | | | | | + 0: no error | +| | | | + -EPERM: Insufficient permission | +| | | | + -EEXIST: Same name policy already deployed | | | | | + -EBADMSG: policy is invalid | | | | | + -ENOMEM: out of memory (OOM) | | | | | + -ERANGE: policy version number overflow | diff --git a/security/ipe/audit.c b/security/ipe/audit.c index f810f7004498..8df307bb2bab 100644 --- a/security/ipe/audit.c +++ b/security/ipe/audit.c @@ -21,7 +21,7 @@ #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ "policy_digest=" IPE_AUDIT_HASH_ALG ":" -#define AUDIT_POLICY_LOAD_NULL_FMT "policy_name=? policy_version=? "\ +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ "policy_digest=?" #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ "old_active_pol_version=%hu.%hu.%hu "\ @@ -255,9 +255,8 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, */ void ipe_audit_policy_load(const struct ipe_policy *const p) { - int res = 0; - int err = 0; struct audit_buffer *ab; + int err = 0; ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_IPE_POLICY_LOAD); @@ -266,15 +265,14 @@ void ipe_audit_policy_load(const struct ipe_policy *const p) if (!IS_ERR(p)) { audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); - res = 1; } else { - audit_log_format(ab, AUDIT_POLICY_LOAD_NULL_FMT); + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); err = PTR_ERR(p); } audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", from_kuid(&init_user_ns, audit_get_loginuid(current)), - audit_get_sessionid(current), res, err); + audit_get_sessionid(current), !err, err); audit_log_end(ab); } diff --git a/security/ipe/fs.c b/security/ipe/fs.c index 5b6d19fb844a..40805b13ee2c 100644 --- a/security/ipe/fs.c +++ b/security/ipe/fs.c @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, char *copy = NULL; int rc = 0; - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) - return -EPERM; + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { + rc = -EPERM; + goto out; + } copy = memdup_user_nul(data, len); - if (IS_ERR(copy)) - return PTR_ERR(copy); + if (IS_ERR(copy)) { + rc = PTR_ERR(copy); + goto out; + } p = ipe_new_policy(NULL, 0, copy, len); if (IS_ERR(p)) { @@ -161,8 +165,11 @@ static ssize_t new_policy(struct file *f, const char __user *data, ipe_audit_policy_load(p); out: - if (rc < 0) + if (rc < 0) { ipe_free_policy(p); + p = ERR_PTR(rc); + ipe_audit_policy_load(p); + } kfree(copy); return (rc < 0) ? rc : len; } diff --git a/security/ipe/policy.c b/security/ipe/policy.c index 0f616e9fbe61..b628f696e32b 100644 --- a/security/ipe/policy.c +++ b/security/ipe/policy.c @@ -202,9 +202,7 @@ struct ipe_policy *ipe_new_policy(const char *text, size_t textlen, return new; err: ipe_free_policy(new); - new = ERR_PTR(rc); - ipe_audit_policy_load(new); - return new; + return ERR_PTR(rc); } /** diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c index 3bcd8cbd09df..74f4e7288331 100644 --- a/security/ipe/policy_fs.c +++ b/security/ipe/policy_fs.c @@ -12,6 +12,7 @@ #include "policy.h" #include "eval.h" #include "fs.h" +#include "audit.h" #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") @@ -288,25 +289,38 @@ static ssize_t getactive(struct file *f, char __user *data, static ssize_t update_policy(struct file *f, const char __user *data, size_t len, loff_t *offset) { + const struct ipe_policy *p = NULL; struct inode *root = NULL; char *copy = NULL; int rc = 0; - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) - return -EPERM; + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { + rc = -EPERM; + goto out; + } copy = memdup_user(data, len); - if (IS_ERR(copy)) - return PTR_ERR(copy); + if (IS_ERR(copy)) { + rc = PTR_ERR(copy); + goto out; + } root = d_inode(f->f_path.dentry->d_parent); inode_lock(root); rc = ipe_update_policy(root, NULL, 0, copy, len); + if (rc < 0) { + inode_unlock(root); + goto out; + } inode_unlock(root); +out: kfree(copy); - if (rc) + if (rc) { + p = ERR_PTR(rc); + ipe_audit_policy_load(p); return rc; + } return len; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ipe: add errno field to IPE policy load auditing 2025-02-27 22:46 ` [PATCH v2] ipe: add errno field to IPE policy load auditing Jasjiv Singh @ 2025-02-27 23:41 ` Fan Wu 2025-02-28 23:11 ` [PATCH v3] " Jasjiv Singh 0 siblings, 1 reply; 9+ messages in thread From: Fan Wu @ 2025-02-27 23:41 UTC (permalink / raw) To: Jasjiv Singh Cc: wufan, audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On Thu, Feb 27, 2025 at 2:46 PM Jasjiv Singh <jasjivsingh@linux.microsoft.com> wrote: > > Thanks for reviewing it. Here's the example generated from real logs: > > AUDIT_IPE_POLICY_LOAD(1422): > audit: AUDIT1422 policy_name="Test_Policy" policy_version=0.0.1 > policy_digest =sha256:84EFBA8FA71E62AE0A537FAB962F8A2BD1053964C4 > 299DCA92BFFF4DB82E86D3 auid=1000 ses=3 lsm=ipe res=1 errno=0 > > The above record shows a new policy has been successfully loaded into > the kernel with the policy name, version, and hash with the errno=0. > > AUDIT_IPE_POLICY_LOAD(1422) with error: > > audit: AUDIT1422 policy_name=? policy_version=? policy_digest=? > auid=1000 ses=3 lsm=ipe res=0 errno=-74 > > The above record shows a policy load failure due to an invalid policy. > > I have updated the failure cases in new_policy() and update_policy(), > which covers each case as well. > > Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> Please merge the old and new changes into one single patch. Also the commit message is supposed to be used to describe the code change. > --- > Documentation/admin-guide/LSM/ipe.rst | 2 ++ > security/ipe/audit.c | 10 ++++------ > security/ipe/fs.c | 17 ++++++++++++----- > security/ipe/policy.c | 4 +--- > security/ipe/policy_fs.c | 24 +++++++++++++++++++----- > 5 files changed, 38 insertions(+), 19 deletions(-) > > diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst > index 2143165f48c9..5dbf54471fab 100644 > --- a/Documentation/admin-guide/LSM/ipe.rst > +++ b/Documentation/admin-guide/LSM/ipe.rst > @@ -453,6 +453,8 @@ Field descriptions: > | errno | integer | No | The result of the policy error as follows: | > | | | | | > | | | | + 0: no error | > +| | | | + -EPERM: Insufficient permission | > +| | | | + -EEXIST: Same name policy already deployed | > | | | | + -EBADMSG: policy is invalid | > | | | | + -ENOMEM: out of memory (OOM) | > | | | | + -ERANGE: policy version number overflow | > diff --git a/security/ipe/audit.c b/security/ipe/audit.c > index f810f7004498..8df307bb2bab 100644 > --- a/security/ipe/audit.c > +++ b/security/ipe/audit.c > @@ -21,7 +21,7 @@ > > #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ > "policy_digest=" IPE_AUDIT_HASH_ALG ":" > -#define AUDIT_POLICY_LOAD_NULL_FMT "policy_name=? policy_version=? "\ > +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ > "policy_digest=?" > #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ > "old_active_pol_version=%hu.%hu.%hu "\ > @@ -255,9 +255,8 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, > */ > void ipe_audit_policy_load(const struct ipe_policy *const p) > { > - int res = 0; > - int err = 0; > struct audit_buffer *ab; > + int err = 0; > > ab = audit_log_start(audit_context(), GFP_KERNEL, > AUDIT_IPE_POLICY_LOAD); > @@ -266,15 +265,14 @@ void ipe_audit_policy_load(const struct ipe_policy *const p) > > if (!IS_ERR(p)) { > audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); > - res = 1; > } else { > - audit_log_format(ab, AUDIT_POLICY_LOAD_NULL_FMT); > + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); > err = PTR_ERR(p); > } > > audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", > from_kuid(&init_user_ns, audit_get_loginuid(current)), > - audit_get_sessionid(current), res, err); > + audit_get_sessionid(current), !err, err); > > audit_log_end(ab); > } > diff --git a/security/ipe/fs.c b/security/ipe/fs.c > index 5b6d19fb844a..40805b13ee2c 100644 > --- a/security/ipe/fs.c > +++ b/security/ipe/fs.c > @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, > char *copy = NULL; > int rc = 0; > > - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) > - return -EPERM; > + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { > + rc = -EPERM; > + goto out; > + } > > copy = memdup_user_nul(data, len); > - if (IS_ERR(copy)) > - return PTR_ERR(copy); > + if (IS_ERR(copy)) { > + rc = PTR_ERR(copy); > + goto out; > + } > > p = ipe_new_policy(NULL, 0, copy, len); > if (IS_ERR(p)) { > @@ -161,8 +165,11 @@ static ssize_t new_policy(struct file *f, const char __user *data, > ipe_audit_policy_load(p); > > out: > - if (rc < 0) > + if (rc < 0) { > ipe_free_policy(p); > + p = ERR_PTR(rc); > + ipe_audit_policy_load(p); How about ipe_audit_policy_load(ERR_PTR(rc)); > + } > kfree(copy); > return (rc < 0) ? rc : len; > } > diff --git a/security/ipe/policy.c b/security/ipe/policy.c > index 0f616e9fbe61..b628f696e32b 100644 > --- a/security/ipe/policy.c > +++ b/security/ipe/policy.c > @@ -202,9 +202,7 @@ struct ipe_policy *ipe_new_policy(const char *text, size_t textlen, > return new; > err: > ipe_free_policy(new); > - new = ERR_PTR(rc); > - ipe_audit_policy_load(new); > - return new; > + return ERR_PTR(rc); > } > > /** > diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c > index 3bcd8cbd09df..74f4e7288331 100644 > --- a/security/ipe/policy_fs.c > +++ b/security/ipe/policy_fs.c > @@ -12,6 +12,7 @@ > #include "policy.h" > #include "eval.h" > #include "fs.h" > +#include "audit.h" > > #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") > > @@ -288,25 +289,38 @@ static ssize_t getactive(struct file *f, char __user *data, > static ssize_t update_policy(struct file *f, const char __user *data, > size_t len, loff_t *offset) > { > + const struct ipe_policy *p = NULL; This var can be avoided. > struct inode *root = NULL; > char *copy = NULL; > int rc = 0; > > - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) > - return -EPERM; > + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { > + rc = -EPERM; > + goto out; > + } > > copy = memdup_user(data, len); > - if (IS_ERR(copy)) > - return PTR_ERR(copy); > + if (IS_ERR(copy)) { > + rc = PTR_ERR(copy); > + goto out; > + } > > root = d_inode(f->f_path.dentry->d_parent); > inode_lock(root); > rc = ipe_update_policy(root, NULL, 0, copy, len); > + if (rc < 0) { > + inode_unlock(root); > + goto out; > + } This part seems unnecessary. > inode_unlock(root); > > +out: > kfree(copy); > - if (rc) > + if (rc) { > + p = ERR_PTR(rc); > + ipe_audit_policy_load(p); p can be avoided, see the above comments. Also please update function documentation if it has a significant change. -Fan ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-02-27 23:41 ` Fan Wu @ 2025-02-28 23:11 ` Jasjiv Singh 2025-03-03 22:11 ` Fan Wu 0 siblings, 1 reply; 9+ messages in thread From: Jasjiv Singh @ 2025-02-28 23:11 UTC (permalink / raw) To: wufan Cc: audit, corbet, eparis, jasjivsingh, jmorris, linux-doc, linux-kernel, linux-security-module, paul Users of IPE require a way to identify when and why an operation fails, allowing them to both respond to violations of policy and be notified of potentially malicious actions on their systems with respect to IPE. This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event to log policy loading failures. Currently, IPE only logs successful policy loads, but not failures. Tracking failures is crucial to detect malicious attempts and ensure a complete audit trail for security events. The new error field will capture the following error codes: * 0: no error * -EPERM: Insufficient permission * -EEXIST: Same name policy already deployed * -EBADMSG: policy is invalid * -ENOMEM: out of memory (OOM) * -ERANGE: policy version number overflow * -EINVAL: policy version parsing error Here are some examples of the updated audit record types: AUDIT_IPE_POLICY_LOAD(1422): audit: AUDIT1422 policy_name="Test_Policy" policy_version=0.0.1 policy_digest=sha256:84EFBA8FA71E62AE0A537FAB962F8A2BD1053964C4299DCA 92BFFF4DB82E86D3 auid=1000 ses=3 lsm=ipe res=1 errno=0 The above record shows a new policy has been successfully loaded into the kernel with the policy name, version, and hash with the errno=0. AUDIT_IPE_POLICY_LOAD(1422) with error: audit: AUDIT1422 policy_name=? policy_version=? policy_digest=? auid=1000 ses=3 lsm=ipe res=0 errno=-74 The above record shows a policy load failure due to an invalid policy (-EBADMSG). By adding this error field, we ensure that all policy load attempts, whether successful or failed, are logged, providing a comprehensive audit trail for IPE policy management. Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> --- Documentation/admin-guide/LSM/ipe.rst | 19 ++++++++++++++----- security/ipe/audit.c | 15 ++++++++++++--- security/ipe/fs.c | 16 +++++++++++----- security/ipe/policy_fs.c | 18 +++++++++++++----- 4 files changed, 50 insertions(+), 18 deletions(-) diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst index f93a467db628..5dbf54471fab 100644 --- a/Documentation/admin-guide/LSM/ipe.rst +++ b/Documentation/admin-guide/LSM/ipe.rst @@ -423,7 +423,7 @@ Field descriptions: Event Example:: - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0 type=1300 audit(1653425529.927:53): arch=c000003e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null) type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E @@ -436,11 +436,11 @@ Field descriptions: +----------------+------------+-----------+---------------------------------------------------+ | Field | Value Type | Optional? | Description of Value | +================+============+===========+===================================================+ -| policy_name | string | No | The policy_name | +| policy_name | string | Yes | The policy_name | +----------------+------------+-----------+---------------------------------------------------+ -| policy_version | string | No | The policy_version | +| policy_version | string | Yes | The policy_version | +----------------+------------+-----------+---------------------------------------------------+ -| policy_digest | string | No | The policy hash | +| policy_digest | string | Yes | The policy hash | +----------------+------------+-----------+---------------------------------------------------+ | auid | integer | No | The login user ID | +----------------+------------+-----------+---------------------------------------------------+ @@ -450,7 +450,16 @@ Field descriptions: +----------------+------------+-----------+---------------------------------------------------+ | res | integer | No | The result of the audited operation(success/fail) | +----------------+------------+-----------+---------------------------------------------------+ - +| errno | integer | No | The result of the policy error as follows: | +| | | | | +| | | | + 0: no error | +| | | | + -EPERM: Insufficient permission | +| | | | + -EEXIST: Same name policy already deployed | +| | | | + -EBADMSG: policy is invalid | +| | | | + -ENOMEM: out of memory (OOM) | +| | | | + -ERANGE: policy version number overflow | +| | | | + -EINVAL: policy version parsing error | ++----------------+------------+-----------+---------------------------------------------------+ 1404 AUDIT_MAC_STATUS ^^^^^^^^^^^^^^^^^^^^^ diff --git a/security/ipe/audit.c b/security/ipe/audit.c index f05f0caa4850..8df307bb2bab 100644 --- a/security/ipe/audit.c +++ b/security/ipe/audit.c @@ -21,6 +21,8 @@ #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ "policy_digest=" IPE_AUDIT_HASH_ALG ":" +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ + "policy_digest=?" #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ "old_active_pol_version=%hu.%hu.%hu "\ "old_policy_digest=" IPE_AUDIT_HASH_ALG ":" @@ -254,16 +256,23 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, void ipe_audit_policy_load(const struct ipe_policy *const p) { struct audit_buffer *ab; + int err = 0; ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_IPE_POLICY_LOAD); if (!ab) return; - audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); - audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1", + if (!IS_ERR(p)) { + audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); + } else { + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); + err = PTR_ERR(p); + } + + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", from_kuid(&init_user_ns, audit_get_loginuid(current)), - audit_get_sessionid(current)); + audit_get_sessionid(current), !err, err); audit_log_end(ab); } diff --git a/security/ipe/fs.c b/security/ipe/fs.c index 5b6d19fb844a..da51264a1d0f 100644 --- a/security/ipe/fs.c +++ b/security/ipe/fs.c @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, char *copy = NULL; int rc = 0; - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) - return -EPERM; + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { + rc = -EPERM; + goto out; + } copy = memdup_user_nul(data, len); - if (IS_ERR(copy)) - return PTR_ERR(copy); + if (IS_ERR(copy)) { + rc = PTR_ERR(copy); + goto out; + } p = ipe_new_policy(NULL, 0, copy, len); if (IS_ERR(p)) { @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, ipe_audit_policy_load(p); out: - if (rc < 0) + if (rc < 0) { ipe_free_policy(p); + ipe_audit_policy_load(ERR_PTR(rc)); + } kfree(copy); return (rc < 0) ? rc : len; } diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c index 3bcd8cbd09df..5f4a8e92bdcf 100644 --- a/security/ipe/policy_fs.c +++ b/security/ipe/policy_fs.c @@ -12,6 +12,7 @@ #include "policy.h" #include "eval.h" #include "fs.h" +#include "audit.h" #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") @@ -292,21 +293,28 @@ static ssize_t update_policy(struct file *f, const char __user *data, char *copy = NULL; int rc = 0; - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) - return -EPERM; + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { + rc = -EPERM; + goto out; + } copy = memdup_user(data, len); - if (IS_ERR(copy)) - return PTR_ERR(copy); + if (IS_ERR(copy)) { + rc = PTR_ERR(copy); + goto out; + } root = d_inode(f->f_path.dentry->d_parent); inode_lock(root); rc = ipe_update_policy(root, NULL, 0, copy, len); inode_unlock(root); +out: kfree(copy); - if (rc) + if (rc) { + ipe_audit_policy_load(ERR_PTR(rc)); return rc; + } return len; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-02-28 23:11 ` [PATCH v3] " Jasjiv Singh @ 2025-03-03 22:11 ` Fan Wu 2025-03-05 0:04 ` Jasjiv Singh 0 siblings, 1 reply; 9+ messages in thread From: Fan Wu @ 2025-03-03 22:11 UTC (permalink / raw) To: Jasjiv Singh Cc: wufan, audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh <jasjivsingh@linux.microsoft.com> wrote: > > Users of IPE require a way to identify when and why an operation fails, > allowing them to both respond to violations of policy and be notified > of potentially malicious actions on their systems with respect to IPE. > > This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event > to log policy loading failures. Currently, IPE only logs successful policy > loads, but not failures. Tracking failures is crucial to detect malicious > attempts and ensure a complete audit trail for security events. > > The new error field will capture the following error codes: > > * 0: no error > * -EPERM: Insufficient permission > * -EEXIST: Same name policy already deployed > * -EBADMSG: policy is invalid > * -ENOMEM: out of memory (OOM) > * -ERANGE: policy version number overflow > * -EINVAL: policy version parsing error > These error codes are not exhaustive. We recently introduced the secondary keyring and platform keyring to sign policy so the policy loading could return -ENOKEY or -EKEYREJECT. And also the update policy can return -ESTALE when the policy version is old. This is my fault that I forgot we should also update the documentation of the newly introduced error codes. Could you please go through the whole loading code and find all possible error codes? Also this is a good chance to update the current stale function documents. ... > > Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> > --- > Documentation/admin-guide/LSM/ipe.rst | 19 ++++++++++++++----- > security/ipe/audit.c | 15 ++++++++++++--- > security/ipe/fs.c | 16 +++++++++++----- > security/ipe/policy_fs.c | 18 +++++++++++++----- > 4 files changed, 50 insertions(+), 18 deletions(-) > > diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst > index f93a467db628..5dbf54471fab 100644 > --- a/Documentation/admin-guide/LSM/ipe.rst > +++ b/Documentation/admin-guide/LSM/ipe.rst > @@ -423,7 +423,7 @@ Field descriptions: > > Event Example:: > > - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 > + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0 > type=1300 audit(1653425529.927:53): arch=c000003e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null) > type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E > > @@ -436,11 +436,11 @@ Field descriptions: > +----------------+------------+-----------+---------------------------------------------------+ > | Field | Value Type | Optional? | Description of Value | > +================+============+===========+===================================================+ > -| policy_name | string | No | The policy_name | > +| policy_name | string | Yes | The policy_name | > +----------------+------------+-----------+---------------------------------------------------+ > -| policy_version | string | No | The policy_version | > +| policy_version | string | Yes | The policy_version | > +----------------+------------+-----------+---------------------------------------------------+ > -| policy_digest | string | No | The policy hash | > +| policy_digest | string | Yes | The policy hash | > +----------------+------------+-----------+---------------------------------------------------+ > | auid | integer | No | The login user ID | > +----------------+------------+-----------+---------------------------------------------------+ > @@ -450,7 +450,16 @@ Field descriptions: > +----------------+------------+-----------+---------------------------------------------------+ > | res | integer | No | The result of the audited operation(success/fail) | > +----------------+------------+-----------+---------------------------------------------------+ > - > +| errno | integer | No | The result of the policy error as follows: | > +| | | | | > +| | | | + 0: no error | > +| | | | + -EPERM: Insufficient permission | > +| | | | + -EEXIST: Same name policy already deployed | > +| | | | + -EBADMSG: policy is invalid | > +| | | | + -ENOMEM: out of memory (OOM) | > +| | | | + -ERANGE: policy version number overflow | > +| | | | + -EINVAL: policy version parsing error | > ++----------------+------------+-----------+---------------------------------------------------+ > Might be better to create another table to list all potential erronos. Also please keep the capitalization of sentences consistent. > 1404 AUDIT_MAC_STATUS > ^^^^^^^^^^^^^^^^^^^^^ > diff --git a/security/ipe/audit.c b/security/ipe/audit.c > index f05f0caa4850..8df307bb2bab 100644 > --- a/security/ipe/audit.c > +++ b/security/ipe/audit.c > @@ -21,6 +21,8 @@ > > #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ > "policy_digest=" IPE_AUDIT_HASH_ALG ":" > +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ > + "policy_digest=?" > #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ > "old_active_pol_version=%hu.%hu.%hu "\ > "old_policy_digest=" IPE_AUDIT_HASH_ALG ":" > @@ -254,16 +256,23 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, > void ipe_audit_policy_load(const struct ipe_policy *const p) > { The documentation of this function should also be updated since it is also auditing errors now. > struct audit_buffer *ab; > + int err = 0; > > ab = audit_log_start(audit_context(), GFP_KERNEL, > AUDIT_IPE_POLICY_LOAD); > if (!ab) > return; > > - audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); > - audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1", > + if (!IS_ERR(p)) { > + audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); > + } else { > + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); > + err = PTR_ERR(p); > + } > + > + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", > from_kuid(&init_user_ns, audit_get_loginuid(current)), > - audit_get_sessionid(current)); > + audit_get_sessionid(current), !err, err); > > audit_log_end(ab); > } > diff --git a/security/ipe/fs.c b/security/ipe/fs.c > index 5b6d19fb844a..da51264a1d0f 100644 > --- a/security/ipe/fs.c > +++ b/security/ipe/fs.c > @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, > char *copy = NULL; > int rc = 0; > > - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) > - return -EPERM; > + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { > + rc = -EPERM; > + goto out; > + } > > copy = memdup_user_nul(data, len); > - if (IS_ERR(copy)) > - return PTR_ERR(copy); > + if (IS_ERR(copy)) { > + rc = PTR_ERR(copy); > + goto out; > + } > > p = ipe_new_policy(NULL, 0, copy, len); > if (IS_ERR(p)) { > @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, > ipe_audit_policy_load(p); > > out: > - if (rc < 0) > + if (rc < 0) { > ipe_free_policy(p); > + ipe_audit_policy_load(ERR_PTR(rc)); > + } > kfree(copy); > return (rc < 0) ? rc : len; > } In case of memdup fail, the kfree(copy) will be called with the error pointer. Also how about refactor the code like ipe_audit_policy_load(p); kfree(copy); return len; err: ipe_audit_policy_load(ERR_PTR(rc)); ipe_free_policy(p); return rc; > diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c > index 3bcd8cbd09df..5f4a8e92bdcf 100644 > --- a/security/ipe/policy_fs.c > +++ b/security/ipe/policy_fs.c > @@ -12,6 +12,7 @@ > #include "policy.h" > #include "eval.h" > #include "fs.h" > +#include "audit.h" > > #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") > > @@ -292,21 +293,28 @@ static ssize_t update_policy(struct file *f, const char __user *data, > char *copy = NULL; > int rc = 0; > > - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) > - return -EPERM; > + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { > + rc = -EPERM; > + goto out; > + } > > copy = memdup_user(data, len); > - if (IS_ERR(copy)) > - return PTR_ERR(copy); > + if (IS_ERR(copy)) { > + rc = PTR_ERR(copy); > + goto out; > + } > > root = d_inode(f->f_path.dentry->d_parent); > inode_lock(root); > rc = ipe_update_policy(root, NULL, 0, copy, len); > inode_unlock(root); > > +out: > kfree(copy); > - if (rc) > + if (rc) { > + ipe_audit_policy_load(ERR_PTR(rc)); > return rc; > + } > The above comments also apply to here. -Fan > return len; > } > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-03-03 22:11 ` Fan Wu @ 2025-03-05 0:04 ` Jasjiv Singh 2025-03-05 18:13 ` Jasjiv Singh 2025-03-05 21:23 ` Fan Wu 0 siblings, 2 replies; 9+ messages in thread From: Jasjiv Singh @ 2025-03-05 0:04 UTC (permalink / raw) To: Fan Wu Cc: audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On 3/3/2025 2:11 PM, Fan Wu wrote: > On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh > <jasjivsingh@linux.microsoft.com> wrote: >> >> Users of IPE require a way to identify when and why an operation fails, >> allowing them to both respond to violations of policy and be notified >> of potentially malicious actions on their systems with respect to IPE. >> >> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event >> to log policy loading failures. Currently, IPE only logs successful policy >> loads, but not failures. Tracking failures is crucial to detect malicious >> attempts and ensure a complete audit trail for security events. >> >> The new error field will capture the following error codes: >> >> * 0: no error >> * -EPERM: Insufficient permission >> * -EEXIST: Same name policy already deployed >> * -EBADMSG: policy is invalid >> * -ENOMEM: out of memory (OOM) >> * -ERANGE: policy version number overflow >> * -EINVAL: policy version parsing error >> > > These error codes are not exhaustive. We recently introduced the > secondary keyring and platform keyring to sign policy so the policy > loading could return -ENOKEY or -EKEYREJECT. And also the update > policy can return -ESTALE when the policy version is old. > This is my fault that I forgot we should also update the documentation > of the newly introduced error codes. Could you please go through the > whole loading code and find all possible error codes? Also this is a > good chance to update the current stale function documents. > > ... > So, I looked into error codes when the policy loads. In ipe_new_policy, the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY, EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same issue with securityfs_create_dir and securityfs_create_file as they return the errno directly from API to. So, what should we return? For other functions: I have complied the errno list: * -ENOENT: Policy is not found while updating * -EEXIST: Same name policy already deployed * -ERANGE: Policy version number overflow * -EINVAL: Policy version parsing error * -EPERM: Insufficient permission * -ESTALE: Policy version is old * -ENOMEM: Out of memory (OOM) * -EBADMSG: Policy is invalid - Jasjiv >> >> Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> >> --- >> Documentation/admin-guide/LSM/ipe.rst | 19 ++++++++++++++----- >> security/ipe/audit.c | 15 ++++++++++++--- >> security/ipe/fs.c | 16 +++++++++++----- >> security/ipe/policy_fs.c | 18 +++++++++++++----- >> 4 files changed, 50 insertions(+), 18 deletions(-) >> >> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst >> index f93a467db628..5dbf54471fab 100644 >> --- a/Documentation/admin-guide/LSM/ipe.rst >> +++ b/Documentation/admin-guide/LSM/ipe.rst >> @@ -423,7 +423,7 @@ Field descriptions: >> >> Event Example:: >> >> - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 >> + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0 >> type=1300 audit(1653425529.927:53): arch=c000003e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null) >> type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E >> >> @@ -436,11 +436,11 @@ Field descriptions: >> +----------------+------------+-----------+---------------------------------------------------+ >> | Field | Value Type | Optional? | Description of Value | >> +================+============+===========+===================================================+ >> -| policy_name | string | No | The policy_name | >> +| policy_name | string | Yes | The policy_name | >> +----------------+------------+-----------+---------------------------------------------------+ >> -| policy_version | string | No | The policy_version | >> +| policy_version | string | Yes | The policy_version | >> +----------------+------------+-----------+---------------------------------------------------+ >> -| policy_digest | string | No | The policy hash | >> +| policy_digest | string | Yes | The policy hash | >> +----------------+------------+-----------+---------------------------------------------------+ >> | auid | integer | No | The login user ID | >> +----------------+------------+-----------+---------------------------------------------------+ >> @@ -450,7 +450,16 @@ Field descriptions: >> +----------------+------------+-----------+---------------------------------------------------+ >> | res | integer | No | The result of the audited operation(success/fail) | >> +----------------+------------+-----------+---------------------------------------------------+ >> - >> +| errno | integer | No | The result of the policy error as follows: | >> +| | | | | >> +| | | | + 0: no error | >> +| | | | + -EPERM: Insufficient permission | >> +| | | | + -EEXIST: Same name policy already deployed | >> +| | | | + -EBADMSG: policy is invalid | >> +| | | | + -ENOMEM: out of memory (OOM) | >> +| | | | + -ERANGE: policy version number overflow | >> +| | | | + -EINVAL: policy version parsing error | >> ++----------------+------------+-----------+---------------------------------------------------+ >> > > Might be better to create another table to list all potential erronos. > Also please keep the capitalization of sentences consistent. > >> 1404 AUDIT_MAC_STATUS >> ^^^^^^^^^^^^^^^^^^^^^ >> diff --git a/security/ipe/audit.c b/security/ipe/audit.c >> index f05f0caa4850..8df307bb2bab 100644 >> --- a/security/ipe/audit.c >> +++ b/security/ipe/audit.c >> @@ -21,6 +21,8 @@ >> >> #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ >> "policy_digest=" IPE_AUDIT_HASH_ALG ":" >> +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ >> + "policy_digest=?" >> #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ >> "old_active_pol_version=%hu.%hu.%hu "\ >> "old_policy_digest=" IPE_AUDIT_HASH_ALG ":" >> @@ -254,16 +256,23 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, >> void ipe_audit_policy_load(const struct ipe_policy *const p) >> { > > The documentation of this function should also be updated since it is > also auditing errors now. > >> struct audit_buffer *ab; >> + int err = 0; >> >> ab = audit_log_start(audit_context(), GFP_KERNEL, >> AUDIT_IPE_POLICY_LOAD); >> if (!ab) >> return; >> >> - audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); >> - audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1", >> + if (!IS_ERR(p)) { >> + audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); >> + } else { >> + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); >> + err = PTR_ERR(p); >> + } >> + >> + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", >> from_kuid(&init_user_ns, audit_get_loginuid(current)), >> - audit_get_sessionid(current)); >> + audit_get_sessionid(current), !err, err); >> >> audit_log_end(ab); >> } >> diff --git a/security/ipe/fs.c b/security/ipe/fs.c >> index 5b6d19fb844a..da51264a1d0f 100644 >> --- a/security/ipe/fs.c >> +++ b/security/ipe/fs.c >> @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, >> char *copy = NULL; >> int rc = 0; >> >> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) >> - return -EPERM; >> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { >> + rc = -EPERM; >> + goto out; >> + } >> >> copy = memdup_user_nul(data, len); >> - if (IS_ERR(copy)) >> - return PTR_ERR(copy); >> + if (IS_ERR(copy)) { >> + rc = PTR_ERR(copy); >> + goto out; >> + } >> >> p = ipe_new_policy(NULL, 0, copy, len); >> if (IS_ERR(p)) { >> @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, >> ipe_audit_policy_load(p); >> >> out: >> - if (rc < 0) >> + if (rc < 0) { >> ipe_free_policy(p); >> + ipe_audit_policy_load(ERR_PTR(rc)); >> + } >> kfree(copy); >> return (rc < 0) ? rc : len; >> } > > In case of memdup fail, the kfree(copy) will be called with the error > pointer. Also how about refactor the code like > > ipe_audit_policy_load(p); > kfree(copy); > > return len; > err: > ipe_audit_policy_load(ERR_PTR(rc)); > ipe_free_policy(p); > > return rc; > >> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c >> index 3bcd8cbd09df..5f4a8e92bdcf 100644 >> --- a/security/ipe/policy_fs.c >> +++ b/security/ipe/policy_fs.c >> @@ -12,6 +12,7 @@ >> #include "policy.h" >> #include "eval.h" >> #include "fs.h" >> +#include "audit.h" >> >> #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") >> >> @@ -292,21 +293,28 @@ static ssize_t update_policy(struct file *f, const char __user *data, >> char *copy = NULL; >> int rc = 0; >> >> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) >> - return -EPERM; >> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { >> + rc = -EPERM; >> + goto out; >> + } >> >> copy = memdup_user(data, len); >> - if (IS_ERR(copy)) >> - return PTR_ERR(copy); >> + if (IS_ERR(copy)) { >> + rc = PTR_ERR(copy); >> + goto out; >> + } >> >> root = d_inode(f->f_path.dentry->d_parent); >> inode_lock(root); >> rc = ipe_update_policy(root, NULL, 0, copy, len); >> inode_unlock(root); >> >> +out: >> kfree(copy); >> - if (rc) >> + if (rc) { >> + ipe_audit_policy_load(ERR_PTR(rc)); >> return rc; >> + } >> > > The above comments also apply to here. > > -Fan > >> return len; >> } >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-03-05 0:04 ` Jasjiv Singh @ 2025-03-05 18:13 ` Jasjiv Singh 2025-03-05 21:23 ` Fan Wu 1 sibling, 0 replies; 9+ messages in thread From: Jasjiv Singh @ 2025-03-05 18:13 UTC (permalink / raw) To: Fan Wu Cc: audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On 3/4/2025 4:04 PM, Jasjiv Singh wrote: > > > On 3/3/2025 2:11 PM, Fan Wu wrote: >> On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh >> <jasjivsingh@linux.microsoft.com> wrote: >>> >>> Users of IPE require a way to identify when and why an operation fails, >>> allowing them to both respond to violations of policy and be notified >>> of potentially malicious actions on their systems with respect to IPE. >>> >>> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event >>> to log policy loading failures. Currently, IPE only logs successful policy >>> loads, but not failures. Tracking failures is crucial to detect malicious >>> attempts and ensure a complete audit trail for security events. >>> >>> The new error field will capture the following error codes: >>> >>> * 0: no error >>> * -EPERM: Insufficient permission >>> * -EEXIST: Same name policy already deployed >>> * -EBADMSG: policy is invalid >>> * -ENOMEM: out of memory (OOM) >>> * -ERANGE: policy version number overflow >>> * -EINVAL: policy version parsing error >>> >> >> These error codes are not exhaustive. We recently introduced the >> secondary keyring and platform keyring to sign policy so the policy >> loading could return -ENOKEY or -EKEYREJECT. And also the update >> policy can return -ESTALE when the policy version is old. >> This is my fault that I forgot we should also update the documentation >> of the newly introduced error codes. Could you please go through the >> whole loading code and find all possible error codes? Also this is a >> good chance to update the current stale function documents. >> >> ... >> > > So, I looked into error codes when the policy loads. In ipe_new_policy, > the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY, > EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions > as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same > issue with securityfs_create_dir and securityfs_create_file as they > return the errno directly from API to. So, what should we return? > > For other functions: I have complied the errno list: > > * -ENOENT: Policy is not found while updating > * -EEXIST: Same name policy already deployed > * -ERANGE: Policy version number overflow > * -EINVAL: Policy version parsing error > * -EPERM: Insufficient permission > * -ESTALE: Policy version is old > * -ENOMEM: Out of memory (OOM) > * -EBADMSG: Policy is invalid > > - Jasjiv > >>> >>> Signed-off-by: Jasjiv Singh <jasjivsingh@linux.microsoft.com> >>> --- >>> Documentation/admin-guide/LSM/ipe.rst | 19 ++++++++++++++----- >>> security/ipe/audit.c | 15 ++++++++++++--- >>> security/ipe/fs.c | 16 +++++++++++----- >>> security/ipe/policy_fs.c | 18 +++++++++++++----- >>> 4 files changed, 50 insertions(+), 18 deletions(-) >>> >>> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst >>> index f93a467db628..5dbf54471fab 100644 >>> --- a/Documentation/admin-guide/LSM/ipe.rst >>> +++ b/Documentation/admin-guide/LSM/ipe.rst >>> @@ -423,7 +423,7 @@ Field descriptions: >>> >>> Event Example:: >>> >>> - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 >>> + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0 >>> type=1300 audit(1653425529.927:53): arch=c000003e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null) >>> type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E >>> >>> @@ -436,11 +436,11 @@ Field descriptions: >>> +----------------+------------+-----------+---------------------------------------------------+ >>> | Field | Value Type | Optional? | Description of Value | >>> +================+============+===========+===================================================+ >>> -| policy_name | string | No | The policy_name | >>> +| policy_name | string | Yes | The policy_name | >>> +----------------+------------+-----------+---------------------------------------------------+ >>> -| policy_version | string | No | The policy_version | >>> +| policy_version | string | Yes | The policy_version | >>> +----------------+------------+-----------+---------------------------------------------------+ >>> -| policy_digest | string | No | The policy hash | >>> +| policy_digest | string | Yes | The policy hash | >>> +----------------+------------+-----------+---------------------------------------------------+ >>> | auid | integer | No | The login user ID | >>> +----------------+------------+-----------+---------------------------------------------------+ >>> @@ -450,7 +450,16 @@ Field descriptions: >>> +----------------+------------+-----------+---------------------------------------------------+ >>> | res | integer | No | The result of the audited operation(success/fail) | >>> +----------------+------------+-----------+---------------------------------------------------+ >>> - >>> +| errno | integer | No | The result of the policy error as follows: | >>> +| | | | | >>> +| | | | + 0: no error | >>> +| | | | + -EPERM: Insufficient permission | >>> +| | | | + -EEXIST: Same name policy already deployed | >>> +| | | | + -EBADMSG: policy is invalid | >>> +| | | | + -ENOMEM: out of memory (OOM) | >>> +| | | | + -ERANGE: policy version number overflow | >>> +| | | | + -EINVAL: policy version parsing error | >>> ++----------------+------------+-----------+---------------------------------------------------+ >>> >> >> Might be better to create another table to list all potential erronos. >> Also please keep the capitalization of sentences consistent. >> >>> 1404 AUDIT_MAC_STATUS >>> ^^^^^^^^^^^^^^^^^^^^^ >>> diff --git a/security/ipe/audit.c b/security/ipe/audit.c >>> index f05f0caa4850..8df307bb2bab 100644 >>> --- a/security/ipe/audit.c >>> +++ b/security/ipe/audit.c >>> @@ -21,6 +21,8 @@ >>> >>> #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\ >>> "policy_digest=" IPE_AUDIT_HASH_ALG ":" >>> +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\ >>> + "policy_digest=?" >>> #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\ >>> "old_active_pol_version=%hu.%hu.%hu "\ >>> "old_policy_digest=" IPE_AUDIT_HASH_ALG ":" >>> @@ -254,16 +256,23 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op, >>> void ipe_audit_policy_load(const struct ipe_policy *const p) >>> { >> >> The documentation of this function should also be updated since it is >> also auditing errors now. >> >>> struct audit_buffer *ab; >>> + int err = 0; >>> >>> ab = audit_log_start(audit_context(), GFP_KERNEL, >>> AUDIT_IPE_POLICY_LOAD); >>> if (!ab) >>> return; >>> >>> - audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); >>> - audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1", >>> + if (!IS_ERR(p)) { >>> + audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p); >>> + } else { >>> + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT); >>> + err = PTR_ERR(p); >>> + } >>> + >>> + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d", >>> from_kuid(&init_user_ns, audit_get_loginuid(current)), >>> - audit_get_sessionid(current)); >>> + audit_get_sessionid(current), !err, err); >>> >>> audit_log_end(ab); >>> } >>> diff --git a/security/ipe/fs.c b/security/ipe/fs.c >>> index 5b6d19fb844a..da51264a1d0f 100644 >>> --- a/security/ipe/fs.c >>> +++ b/security/ipe/fs.c >>> @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, >>> char *copy = NULL; >>> int rc = 0; >>> >>> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) >>> - return -EPERM; >>> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { >>> + rc = -EPERM; >>> + goto out; >>> + } >>> >>> copy = memdup_user_nul(data, len); >>> - if (IS_ERR(copy)) >>> - return PTR_ERR(copy); >>> + if (IS_ERR(copy)) { >>> + rc = PTR_ERR(copy); >>> + goto out; >>> + } >>> >>> p = ipe_new_policy(NULL, 0, copy, len); >>> if (IS_ERR(p)) { >>> @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, >>> ipe_audit_policy_load(p); >>> >>> out: >>> - if (rc < 0) >>> + if (rc < 0) { >>> ipe_free_policy(p); >>> + ipe_audit_policy_load(ERR_PTR(rc)); >>> + } >>> kfree(copy); >>> return (rc < 0) ? rc : len; >>> } >> >> In case of memdup fail, the kfree(copy) will be called with the error >> pointer. Also how about refactor the code like >> >> ipe_audit_policy_load(p); >> kfree(copy); >> >> return len; >> err: >> ipe_audit_policy_load(ERR_PTR(rc)); >> ipe_free_policy(p); >> >> return rc; Another issue I was thinking about that is what if memdup works but then ipe_new_policy fails, then we still need to call kfree but the above code mentioned by you will not do that. >> >>> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c >>> index 3bcd8cbd09df..5f4a8e92bdcf 100644 >>> --- a/security/ipe/policy_fs.c >>> +++ b/security/ipe/policy_fs.c >>> @@ -12,6 +12,7 @@ >>> #include "policy.h" >>> #include "eval.h" >>> #include "fs.h" >>> +#include "audit.h" >>> >>> #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535") >>> >>> @@ -292,21 +293,28 @@ static ssize_t update_policy(struct file *f, const char __user *data, >>> char *copy = NULL; >>> int rc = 0; >>> >>> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) >>> - return -EPERM; >>> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { >>> + rc = -EPERM; >>> + goto out; >>> + } >>> >>> copy = memdup_user(data, len); >>> - if (IS_ERR(copy)) >>> - return PTR_ERR(copy); >>> + if (IS_ERR(copy)) { >>> + rc = PTR_ERR(copy); >>> + goto out; >>> + } >>> >>> root = d_inode(f->f_path.dentry->d_parent); >>> inode_lock(root); >>> rc = ipe_update_policy(root, NULL, 0, copy, len); >>> inode_unlock(root); >>> >>> +out: >>> kfree(copy); >>> - if (rc) >>> + if (rc) { >>> + ipe_audit_policy_load(ERR_PTR(rc)); >>> return rc; >>> + } >>> >> >> The above comments also apply to here. >> >> -Fan >> >>> return len; >>> } >>> -- >>> 2.34.1 >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-03-05 0:04 ` Jasjiv Singh 2025-03-05 18:13 ` Jasjiv Singh @ 2025-03-05 21:23 ` Fan Wu 2025-03-05 23:27 ` Jasjiv Singh 1 sibling, 1 reply; 9+ messages in thread From: Fan Wu @ 2025-03-05 21:23 UTC (permalink / raw) To: Jasjiv Singh Cc: Fan Wu, audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh <jasjivsingh@linux.microsoft.com> wrote: > > > > On 3/3/2025 2:11 PM, Fan Wu wrote: > > On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh > > <jasjivsingh@linux.microsoft.com> wrote: > >> > >> Users of IPE require a way to identify when and why an operation fails, > >> allowing them to both respond to violations of policy and be notified > >> of potentially malicious actions on their systems with respect to IPE. > >> > >> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event > >> to log policy loading failures. Currently, IPE only logs successful policy > >> loads, but not failures. Tracking failures is crucial to detect malicious > >> attempts and ensure a complete audit trail for security events. > >> > >> The new error field will capture the following error codes: > >> > >> * 0: no error > >> * -EPERM: Insufficient permission > >> * -EEXIST: Same name policy already deployed > >> * -EBADMSG: policy is invalid > >> * -ENOMEM: out of memory (OOM) > >> * -ERANGE: policy version number overflow > >> * -EINVAL: policy version parsing error > >> > > > > These error codes are not exhaustive. We recently introduced the > > secondary keyring and platform keyring to sign policy so the policy > > loading could return -ENOKEY or -EKEYREJECT. And also the update > > policy can return -ESTALE when the policy version is old. > > This is my fault that I forgot we should also update the documentation > > of the newly introduced error codes. Could you please go through the > > whole loading code and find all possible error codes? Also this is a > > good chance to update the current stale function documents. > > > > ... > > > > So, I looked into error codes when the policy loads. In ipe_new_policy, > the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY, > EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions > as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same > issue with securityfs_create_dir and securityfs_create_file as they > return the errno directly from API to. So, what should we return? I think the key here is we need to document the errors in the ipe's context. For example, ENOKEY means the key used to sign the ipe policy is not found in the keyring, EKEYREJECTED means ipe signature verification failed. If an error does not have specific meaning for ipe then probably we don't need to document it. > > For other functions: I have complied the errno list: > > * -ENOENT: Policy is not found while updating This one means policy was deleted while updating, this only happens the update happened just after the policy deletion. > * -EEXIST: Same name policy already deployed > * -ERANGE: Policy version number overflow > * -EINVAL: Policy version parsing error > * -EPERM: Insufficient permission > * -ESTALE: Policy version is old Maybe make this one clearer, how about trying to update an ipe policy with an older version policy. -Fan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-03-05 21:23 ` Fan Wu @ 2025-03-05 23:27 ` Jasjiv Singh 2025-03-06 0:08 ` Fan Wu 0 siblings, 1 reply; 9+ messages in thread From: Jasjiv Singh @ 2025-03-05 23:27 UTC (permalink / raw) To: Fan Wu Cc: audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On 3/5/2025 1:23 PM, Fan Wu wrote: > On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh > <jasjivsingh@linux.microsoft.com> wrote: >> >> >> >> On 3/3/2025 2:11 PM, Fan Wu wrote: >>> On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh >>> <jasjivsingh@linux.microsoft.com> wrote: >>>> >>>> Users of IPE require a way to identify when and why an operation fails, >>>> allowing them to both respond to violations of policy and be notified >>>> of potentially malicious actions on their systems with respect to IPE. >>>> >>>> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event >>>> to log policy loading failures. Currently, IPE only logs successful policy >>>> loads, but not failures. Tracking failures is crucial to detect malicious >>>> attempts and ensure a complete audit trail for security events. >>>> >>>> The new error field will capture the following error codes: >>>> >>>> * 0: no error >>>> * -EPERM: Insufficient permission >>>> * -EEXIST: Same name policy already deployed >>>> * -EBADMSG: policy is invalid >>>> * -ENOMEM: out of memory (OOM) >>>> * -ERANGE: policy version number overflow >>>> * -EINVAL: policy version parsing error >>>> >>> >>> These error codes are not exhaustive. We recently introduced the >>> secondary keyring and platform keyring to sign policy so the policy >>> loading could return -ENOKEY or -EKEYREJECT. And also the update >>> policy can return -ESTALE when the policy version is old. >>> This is my fault that I forgot we should also update the documentation >>> of the newly introduced error codes. Could you please go through the >>> whole loading code and find all possible error codes? Also this is a >>> good chance to update the current stale function documents. >>> >>> ... >>> >> >> So, I looked into error codes when the policy loads. In ipe_new_policy, >> the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY, >> EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions >> as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same >> issue with securityfs_create_dir and securityfs_create_file as they >> return the errno directly from API to. So, what should we return? > > I think the key here is we need to document the errors in the ipe's > context. For example, ENOKEY means the key used to sign the ipe policy > is not found in the keyring, EKEYREJECTED means ipe signature > verification failed. If an error does not have specific meaning for > ipe then probably we don't need to document it. > >> >> For other functions: I have complied the errno list: >> >> * -ENOENT: Policy is not found while updating > > This one means policy was deleted while updating, this only happens > the update happened just after the policy deletion. > >> * -EEXIST: Same name policy already deployed >> * -ERANGE: Policy version number overflow >> * -EINVAL: Policy version parsing error >> * -EPERM: Insufficient permission >> * -ESTALE: Policy version is old > > Maybe make this one clearer, how about trying to update an ipe policy > with an older version policy. > > -Fan Thanks, I have compiled the list based on your comments. * -ENOKEY: Key used to sign the IPE policy not found in the keyring * -ESTALE: Attempting to update an IPE policy with an older version * -EKEYREJECTED: IPE signature verification failed * -ENOENT: Policy was deleted while updating * -EEXIST: Same name policy already deployed * -ERANGE: Policy version number overflow * -EINVAL: Policy version parsing error * -EPERM: Insufficient permission * -ENOMEM: Out of memory (OOM) * -EBADMSG: Policy is invalid How does that that look? I will update the documentation with this list in the next patch along with suggestions you mentioned. Moving the memdup failure discussion here: >>> diff --git a/security/ipe/fs.c b/security/ipe/fs.c >>> index 5b6d19fb844a..da51264a1d0f 100644 >>> --- a/security/ipe/fs.c >>> +++ b/security/ipe/fs.c >>> @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, >>> char *copy = NULL; >>> int rc = 0; >>> >>> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) >>> - return -EPERM; >>> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { >>> + rc = -EPERM; >>> + goto out; >>> + } >>> >>> copy = memdup_user_nul(data, len); >>> - if (IS_ERR(copy)) >>> - return PTR_ERR(copy); >>> + if (IS_ERR(copy)) { >>> + rc = PTR_ERR(copy); >>> + goto out; >>> + } >>> >>> p = ipe_new_policy(NULL, 0, copy, len); >>> if (IS_ERR(p)) { >>> @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, >>> ipe_audit_policy_load(p); >>> >>> out: >>> - if (rc < 0) >>> + if (rc < 0) { >>> ipe_free_policy(p); >>> + ipe_audit_policy_load(ERR_PTR(rc)); >>> + } >>> kfree(copy); >>> return (rc < 0) ? rc : len; >>> } >> >> In case of memdup fail, the kfree(copy) will be called with the error >> pointer. Also how about refactor the code like >> >> ipe_audit_policy_load(p); >> kfree(copy); >> >> return len; >> err: >> ipe_audit_policy_load(ERR_PTR(rc)); >> ipe_free_policy(p); >> >> return rc; Another issue I was thinking about that is what if memdup works but then ipe_new_policy fails, then we still need to call kfree but the above code mentioned by you will not do that. I think we can just use "copy = NULL;" after recording the rc value from it, instead of the suggested code. For examples, we can look at selinux. -Jasjiv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] ipe: add errno field to IPE policy load auditing 2025-03-05 23:27 ` Jasjiv Singh @ 2025-03-06 0:08 ` Fan Wu 0 siblings, 0 replies; 9+ messages in thread From: Fan Wu @ 2025-03-06 0:08 UTC (permalink / raw) To: Jasjiv Singh Cc: Fan Wu, audit, corbet, eparis, jmorris, linux-doc, linux-kernel, linux-security-module, paul On Wed, Mar 5, 2025 at 3:27 PM Jasjiv Singh <jasjivsingh@linux.microsoft.com> wrote: > > > > On 3/5/2025 1:23 PM, Fan Wu wrote: > > On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh > > <jasjivsingh@linux.microsoft.com> wrote: > >> > >> > >> > >> On 3/3/2025 2:11 PM, Fan Wu wrote: > >>> On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh > >>> <jasjivsingh@linux.microsoft.com> wrote: > >>>> > >>>> Users of IPE require a way to identify when and why an operation fails, > >>>> allowing them to both respond to violations of policy and be notified > >>>> of potentially malicious actions on their systems with respect to IPE. > >>>> > >>>> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event > >>>> to log policy loading failures. Currently, IPE only logs successful policy > >>>> loads, but not failures. Tracking failures is crucial to detect malicious > >>>> attempts and ensure a complete audit trail for security events. > >>>> > >>>> The new error field will capture the following error codes: > >>>> > >>>> * 0: no error > >>>> * -EPERM: Insufficient permission > >>>> * -EEXIST: Same name policy already deployed > >>>> * -EBADMSG: policy is invalid > >>>> * -ENOMEM: out of memory (OOM) > >>>> * -ERANGE: policy version number overflow > >>>> * -EINVAL: policy version parsing error > >>>> > >>> > >>> These error codes are not exhaustive. We recently introduced the > >>> secondary keyring and platform keyring to sign policy so the policy > >>> loading could return -ENOKEY or -EKEYREJECT. And also the update > >>> policy can return -ESTALE when the policy version is old. > >>> This is my fault that I forgot we should also update the documentation > >>> of the newly introduced error codes. Could you please go through the > >>> whole loading code and find all possible error codes? Also this is a > >>> good chance to update the current stale function documents. > >>> > >>> ... > >>> > >> > >> So, I looked into error codes when the policy loads. In ipe_new_policy, > >> the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY, > >> EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions > >> as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same > >> issue with securityfs_create_dir and securityfs_create_file as they > >> return the errno directly from API to. So, what should we return? > > > > I think the key here is we need to document the errors in the ipe's > > context. For example, ENOKEY means the key used to sign the ipe policy > > is not found in the keyring, EKEYREJECTED means ipe signature > > verification failed. If an error does not have specific meaning for > > ipe then probably we don't need to document it. > > > >> > >> For other functions: I have complied the errno list: > >> > >> * -ENOENT: Policy is not found while updating > > > > This one means policy was deleted while updating, this only happens > > the update happened just after the policy deletion. > > > >> * -EEXIST: Same name policy already deployed > >> * -ERANGE: Policy version number overflow > >> * -EINVAL: Policy version parsing error > >> * -EPERM: Insufficient permission > >> * -ESTALE: Policy version is old > > > > Maybe make this one clearer, how about trying to update an ipe policy > > with an older version policy. > > > > -Fan > > Thanks, I have compiled the list based on your comments. > > * -ENOKEY: Key used to sign the IPE policy not found in the keyring > * -ESTALE: Attempting to update an IPE policy with an older version > * -EKEYREJECTED: IPE signature verification failed > * -ENOENT: Policy was deleted while updating > * -EEXIST: Same name policy already deployed > * -ERANGE: Policy version number overflow > * -EINVAL: Policy version parsing error > * -EPERM: Insufficient permission > * -ENOMEM: Out of memory (OOM) > * -EBADMSG: Policy is invalid > > How does that that look? I will update the documentation with this list > in the next patch along with suggestions you mentioned. > This looks good to me, make sure to also update the function documentations in the code. > > Moving the memdup failure discussion here: > > >>> diff --git a/security/ipe/fs.c b/security/ipe/fs.c > >>> index 5b6d19fb844a..da51264a1d0f 100644 > >>> --- a/security/ipe/fs.c > >>> +++ b/security/ipe/fs.c > >>> @@ -141,12 +141,16 @@ static ssize_t new_policy(struct file *f, const char __user *data, > >>> char *copy = NULL; > >>> int rc = 0; > >>> > >>> - if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) > >>> - return -EPERM; > >>> + if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) { > >>> + rc = -EPERM; > >>> + goto out; > >>> + } > >>> > >>> copy = memdup_user_nul(data, len); > >>> - if (IS_ERR(copy)) > >>> - return PTR_ERR(copy); > >>> + if (IS_ERR(copy)) { > >>> + rc = PTR_ERR(copy); > >>> + goto out; > >>> + } > >>> > >>> p = ipe_new_policy(NULL, 0, copy, len); > >>> if (IS_ERR(p)) { > >>> @@ -161,8 +165,10 @@ static ssize_t new_policy(struct file *f, const char __user *data, > >>> ipe_audit_policy_load(p); > >>> > >>> out: > >>> - if (rc < 0) > >>> + if (rc < 0) { > >>> ipe_free_policy(p); > >>> + ipe_audit_policy_load(ERR_PTR(rc)); > >>> + } > >>> kfree(copy); > >>> return (rc < 0) ? rc : len; > >>> } > >> > >> In case of memdup fail, the kfree(copy) will be called with the error > >> pointer. Also how about refactor the code like > >> > >> ipe_audit_policy_load(p); > >> kfree(copy); > >> > >> return len; > >> err: > >> ipe_audit_policy_load(ERR_PTR(rc)); > >> ipe_free_policy(p); > >> > >> return rc; > > Another issue I was thinking about that is what if memdup works but then > ipe_new_policy fails, then we still need to call kfree but the above code > mentioned by you will not do that. > > I think we can just use "copy = NULL;" after recording the rc value from it, > instead of the suggested code. For examples, we can look at selinux. > > -Jasjiv > Yep this makes sense to me. We can just add "copy = NULL" and still use only one out: tag. -Fan ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-06 0:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAKtyLkFg2+8ciy4DM=g+vcTVvuRPNL2SHbN+m9ObErxtYXZYPw@mail.gmail.com>
2025-02-27 22:46 ` [PATCH v2] ipe: add errno field to IPE policy load auditing Jasjiv Singh
2025-02-27 23:41 ` Fan Wu
2025-02-28 23:11 ` [PATCH v3] " Jasjiv Singh
2025-03-03 22:11 ` Fan Wu
2025-03-05 0:04 ` Jasjiv Singh
2025-03-05 18:13 ` Jasjiv Singh
2025-03-05 21:23 ` Fan Wu
2025-03-05 23:27 ` Jasjiv Singh
2025-03-06 0:08 ` Fan Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox