Re: [PATCH v1 1/2] audit: record fanotify event regardless of presence of rules
On Fri 07-03-25 14:19:38, Richard Guy Briggs wrote:
> On 2025-03-07 15:52, Jan Kara wrote:
> > On Thu 06-03-25 20:12:23, Richard Guy Briggs wrote:
> > > On 2025-03-06 16:06, Jan Kara wrote:
> > > > On Wed 05-03-25 16:33:19, Richard Guy Briggs wrote:
> > > > > When no audit rules are in place, fanotify event results are
> > > > > unconditionally dropped due to an explicit check for the existence of
> > > > > any audit rules. Given this is a report from another security
> > > > > sub-system, allow it to be recorded regardless of the existence of any
> > > > > audit rules.
> > > > >
> > > > > To test, install and run the fapolicyd daemon with default config.
> > > > > Then
> > > > > as an unprivileged user, create and run a very simple binary that
> > > > > should
> > > > > be denied. Then check for an event with
> > > > > ausearch -m FANOTIFY -ts recent
> > > > >
> > > > > Link: https://issues.redhat.com/browse/RHEL-1367
> > > > > Signed-off-by: Richard Guy Briggs
> > > >
> > > > I don't know enough about security modules to tell whether this is what
> > > > admins want or not so that's up to you but:
> > > >
> > > > > -static inline void audit_fanotify(u32 response, struct
> > > > > fanotify_response_info_audit_rule *friar)
> > > > > -{
> > > > > - if (!audit_dummy_context())
> > > > > - __audit_fanotify(response, friar);
> > > > > -}
> > > > > -
> > > >
> > > > I think this is going to break compilation with !CONFIG_AUDITSYSCALL &&
> > > > CONFIG_FANOTIFY?
> > >
> > > Why would that break it? The part of the patch you (prematurely)
> > > deleted takes care of that.
> >
> > So I'm failing to see how it takes care of that when with
> > !CONFIG_AUDITSYSCALL kernel/auditsc.c does not get compiled into the kernel.
> > So what does provide the implementation of audit_fanotify() in that case?
> > I think you need to provide empty audit_fanotify() inline wrapper for that
> > case...
>
> I'm sorry, I responded too quickly without thinking about your question,
> my mistake. It isn't the prototype that was changed in the
> CONFIG_SYSCALL case that is relevant in that case.
>
> There was already in existance in the !CONFIG_AUDITSYSCALL case the
> inline wrapper to do that job:
>
> static inline void audit_fanotify(u32 response, struct
> fanotify_response_info_audit_rule *friar)
> { }
>
> Did I understand correctly this time and does this answer your question?
Yes, thanks for explanation and sorry for not noticing the second
audit_fanotify() implementation. Somehow I've hasted to a conclusion (based
on customs of parts of kernel I maintain ;)) that you rely on
audit_dummy_context() being constant 0 for !CONFIG_AUDITSYSCALL and thus
__audit_fanotify() call getting compiled out (which would not be the case
after your changes).
Anyway, for the patch feel free to add:
Acked-by: Jan Kara
> But you do cause me to notice the case that these notifications will be
> dropped when CONFIG_AUDIT && !CONFIG_AUDITSYSCALL.
Glad my blindness helped something ;)
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH RFC v4 1/1] ipe: add errno field to IPE policy load auditing
On Mar 7, 2025 Jasjiv Singh 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:
>
> * -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
>
> 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).
>
> Signed-off-by: Jasjiv Singh
> ---
> Documentation/admin-guide/LSM/ipe.rst | 69 +++
> security/ipe/audit.c | 21 ++--
> security/ipe/fs.c | 19 ++--
> security/ipe/policy.c | 11 -
> security/ipe/policy_fs.c | 29 ---
> 5 files changed, 111 insertions(+), 38 deletions(-)
...
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index f05f0caa4850..ac9d68b68b8b 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=?"
This should probably be AUDIT_POLICY_LOAD_NULL_FMT to be consistent with
the other IPE audit format macros, e.g. AUDIT_OLD_ACTIVE_POLICY_NULL_FMT.
> diff --git a/security/ipe/fs.c b/security/ipe/fs.c
> index 5b6d19fb844a..db18636470bf 100644
> --- a/security/ipe/fs.c
> +++ b/security/ipe/fs.c
> @@ -133,6 +133,8 @@ static ssize_t getenforce(struct file *f, char __user
> *data,
> * * %-ERANGE- Policy version number overflow
> * * %-EINVAL- Policy version parsing error
> * * %-EEXIST- Same name policy already deployed
> + * * %-ENOKEY- Key used to sign the IPE policy not
> found in the keyring
> + * * %-EKEYREJECTED - IPE signature verification failed
> */
> static ssize_t new_policy(struct file *f, const char __user *data,
> size_t len, loff_t *offset)
> @@ -141,12 +143,17 @@ 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);
> + copy = NULL;
> + goto out;
> + }
>
> p = ipe_new_policy(NULL, 0, copy, len);
> if (IS_ERR(p)) {
> @@ -161,8 +168,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;
> }
I'm going to suggest putting the audit calls closer together to help
ease maintainence, e.g.:
out:
if (rc) {
ipe_audit_policy_load(ERR_PTR(rc));
ipe_free_policy(p);
} else
ipe_audit_policy_load(p);
> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
> index 3bcd8cbd09df..b70d2518b182 100644
> --- a/security/ipe/policy_fs.c
> +++ b/security/ipe/policy_fs.c
> @
[PATCH] fs: dodge an atomic in putname if ref == 1
While the structure is refcounted, the only consumer incrementing it is
audit and even then the atomic operation is only needed when it
interacts with io_uring.
If putname spots a count of 1, there is no legitimate way for anyone to
bump it.
If audit is disabled, the count is guaranteed to be 1, which
consistently elides the atomic for all path lookups. If audit is
enabled, it still manages to elide the last decrement.
Note the patch does not do anything to prevent audit from suffering
atomics. See [1] and [2] for a different approach.
Benchmarked on Sapphire Rapids issuing access() (ops/s):
before: 5106246
after: 5269678 (+3%)
Link 1:
https://lore.kernel.org/linux-fsdevel/20250307161155.760949-1-mjgu...@gmail.com/
Link 2: https://lore.kernel.org/linux-fsdevel/20250307164216.GI2023217@ZenIV/
Signed-off-by: Mateusz Guzik
---
This is an alternative to the patch I linked above.
I think the improved commit message should also cover the feedback
Christian previously shared concerning it.
This is a trivial win until the atomic issue gets resolved, I don't see
any reason to NOT include it. At the same time I don't have that much
interest arguing about it either.
That is to say, here is a different take, if you don't like it, I'm
dropping the subject.
cheers
fs/namei.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 06765d320e7e..add90981cfcd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -275,14 +275,19 @@ EXPORT_SYMBOL(getname_kernel);
void putname(struct filename *name)
{
+ int refcnt;
+
if (IS_ERR_OR_NULL(name))
return;
- if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
- return;
+ refcnt = atomic_read(&name->refcnt);
+ if (refcnt != 1) {
+ if (WARN_ON_ONCE(!refcnt))
+ return;
- if (!atomic_dec_and_test(&name->refcnt))
- return;
+ if (!atomic_dec_and_test(&name->refcnt))
+ return;
+ }
if (name->name != name->iname) {
__putname(name->name);
--
2.43.0
