Re: [PATCH v2] kernel/audit: reorder the member of audit_context

Re: [PATCH v2] kernel/audit: reorder the member of audit_context

[Paul Moore]

On Dec  5, 2024 15074444048@163.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@kylinos.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