linux-hardening - Re: [PATCH v2] kernel/audit: reorder the member of audit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <aef181673159ac72f1e7922548186fc6@paul-moore.com>
Date: Thu, 05 Dec 2024 19:45:04 -0500
From: Paul Moore <paul@...l-moore.com>
To: 15074444048@....com, audit@...r.kernel.org, eparis@...hat.com
Cc: 15074444048@....com, lihaojie <lihaojie@...inos.cn>, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] kernel/audit: reorder the member of audit_context

On Dec  5, 2024 15074444048@....com wrote:
> 
> Adjust the positions of target_ref and target_comm to
> reduce their direct proximity. This can prevent the memset
> operation on target_ref from potentially affecting target_comm

I don't really see the above as very convinving, sure by moving the
target_ref field after target_comm it is less likely that a too-long
memset() on target_ref will impact target_comm, but ...

* lsm_prop is a fixed sized struct so memset()'ing it should be
easy to verify.

* if a memset(target_ref) does go long, it still jeapordizes other
parts of the audit_context struct (see the audit_tree_refs pointers).

If we need to workaround some shortcomings of the static analysis
tooling (which compiler/tool are you using?) that's okay, and the
paragraph below is fine, but I don't believe this change really offers
much in the way of runtime protection (the first paragraph).

I'm CC'ing the linux-hardening folks as they may have some additional
insight on how to best work around static analysis limitations.

> The compiler's static analysis tool considers that the memset
> operation might affect the adjacent struct member target_comm,
> potentially causing subsequent writes to target_comm to exceed
> its boundaries. Even if an overflow does not actually occur,
> the compiler may issue a warning because it cannot fully
> determine the safety of the operation.
> 
> Verification:
> - "pahole" shows no size nor member offset changes to struct audit_context.
> - "objdump -d" shows no object code changes.
> 
> Signed-off-by: lihaojie <lihaojie@...inos.cn>
> ---
>  kernel/audit.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 0211cb307d30..77c140228a4a 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -144,8 +144,8 @@ struct audit_context {
>  	kuid_t		    target_auid;
>  	kuid_t		    target_uid;
>  	unsigned int	    target_sessionid;
> -	struct lsm_prop	    target_ref;
>  	char		    target_comm[TASK_COMM_LEN];
> +	struct lsm_prop	    target_ref;
>  
>  	struct audit_tree_refs *trees, *first_trees;
>  	struct list_head killed_trees;
> -- 
> 2.25.1

--
paul-moore.com