lsm: add reserved flag in lsm_prop struct

Commit Message

李豪杰 Dec. 6, 2024, 11:41 a.m. UTC

From: lihaojie <[e-mail]>

lsm_prop size is controled by macro, lsm_prop size will be 0
when marco don't define. add flag to alloc sm_prop basic size.

empty struct will make target_ref & target_comm in audit_context
located at the same address, __member_size of target_comm is
same as __member_size of target_ref, so strscpy warn buffer
overflow when compile time.

Signed-off-by: lihaojie <[e-mail]>
---
 include/linux/security.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Casey Schaufler Dec. 6, 2024, 5:31 p.m. UTC | #1

On 12/6/2024 3:41 AM, [e-mail] wrote:
> From: lihaojie <[e-mail]>
>
> lsm_prop size is controled by macro, lsm_prop size will be 0
> when marco don't define. add flag to alloc sm_prop basic size.
>
> empty struct will make target_ref & target_comm in audit_context
> located at the same address, __member_size of target_comm is
> same as __member_size of target_ref, so strscpy warn buffer
> overflow when compile time.

Can you cite where this warning occurs?

>
> Signed-off-by: lihaojie <[e-mail]>
> ---
>  include/linux/security.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index cbdba435b798..f502deecb142 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -164,6 +164,7 @@ struct lsm_prop {
>  	struct lsm_prop_smack smack;
>  	struct lsm_prop_apparmor apparmor;
>  	struct lsm_prop_bpf bpf;
> +	u8 reserved;
>  };

I don't care much for this approach. Increasing the size of the structure
to avoid a warning in the case where it isn't used seems problematic.

>  
>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];

李豪杰 Dec. 9, 2024, 1:52 a.m. UTC | #2

At 2024-12-07 01:31:11, "Casey Schaufler" <[e-mail]> wrote:

>
>Can you cite where this warning occurs?
>

In file included from ./include/linux/string.h:389,
                 from ./include/linux/bitmap.h:13,
                 from ./include/linux/cpumask.h:12,
                 from ./include/linux/smp.h:13,
                 from ./include/linux/lockdep.h:14,
                 from ./include/linux/spinlock.h:63,
                 from ./include/linux/wait.h:9,
                 from ./include/linux/wait_bit.h:8,
                 from ./include/linux/fs.h:6,
                 from kernel/auditsc.c:37:
In function ‘sized_strscpy’,
    inlined from ‘__audit_ptrace’ at kernel/auditsc.c:2732:2:
./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
  293 |   __write_overflow();
      |   ^~~~~~~~~~~~~~~~~~
In function ‘sized_strscpy’,
    inlined from ‘audit_signal_info_syscall’ at kernel/auditsc.c:2759:3:
./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
  293 |   __write_overflow();



>
>I don't care much for this approach. Increasing the size of the structure
>to avoid a warning in the case where it isn't used seems problematic.
>

do you have any good sugestion?

李豪杰 Dec. 13, 2024, 8:11 a.m. UTC | #3

Hi paul,


it's the compile error that i met.
In file included from ./include/linux/string.h:389,
                 from ./include/linux/bitmap.h:13,
                 from ./include/linux/cpumask.h:12,
                 from ./include/linux/smp.h:13,
                 from ./include/linux/lockdep.h:14,
                 from ./include/linux/spinlock.h:63,
                 from ./include/linux/wait.h:9,
                 from ./include/linux/wait_bit.h:8,
                 from ./include/linux/fs.h:6,
                 from kernel/auditsc.c:37:
In function ‘sized_strscpy’,
    inlined from ‘__audit_ptrace’ at kernel/auditsc.c:2732:2:
./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
  293 |   __write_overflow();
      |   ^~~~~~~~~~~~~~~~~~
In function ‘sized_strscpy’,
    inlined from ‘audit_signal_info_syscall’ at kernel/auditsc.c:2759:3:
./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
  293 |   __write_overflow();

i see commit d9381508ea2b590aff46d28d432d20bfef1ba64c merged,
but it's a workaround, so how about my thoughts below?

look forward to your reply


At 2024-12-06 19:41:08, [e-mail] wrote:
>From: lihaojie <[e-mail]>
>
>lsm_prop size is controled by macro, lsm_prop size will be 0
>when marco don't define. add flag to alloc sm_prop basic size.
>
>empty struct will make target_ref & target_comm in audit_context
>located at the same address, __member_size of target_comm is
>same as __member_size of target_ref, so strscpy warn buffer
>overflow when compile time.
>
>Signed-off-by: lihaojie <[e-mail]>
>---
> include/linux/security.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/include/linux/security.h b/include/linux/security.h
>index cbdba435b798..f502deecb142 100644
>--- a/include/linux/security.h
>+++ b/include/linux/security.h
>@@ -164,6 +164,7 @@ struct lsm_prop {
> 	struct lsm_prop_smack smack;
> 	struct lsm_prop_apparmor apparmor;
> 	struct lsm_prop_bpf bpf;
>+	u8 reserved;
> };
> 
> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>-- 
>2.25.1
>
>Hi paul
>
>
>The root cause is that target_ref is empty, that make __member_size of target_comm will 
>return target_ref size, because they have same address, so, overflow will be detected.
>as you said, I should not change member size, there may be other potential hazards.
>
>so i add patch v3, please check above. 
>
>I have been test local.
>-gcc will alloc empty struct 0 byte. __member_size of member behind empty struct will return 0 byte;
>
>-g++ will alloc empty struct 1 byte. __member_size of member behind empty struct will return normal bytes; 
>
>-pahole result
>        struct lsm_prop            target_ref;           /*   812     1 */
>        char                       target_comm[16];      /*   813    16 */
>
>
>thanks
>lihaojie

Paul Moore Dec. 18, 2024, 9:24 p.m. UTC | #4

On Fri, Dec 13, 2024 at 3:11 AM 李豪杰 <[e-mail]> wrote:
>
> it's the compile error that i met.
> In file included from ./include/linux/string.h:389,
>                  from ./include/linux/bitmap.h:13,
>                  from ./include/linux/cpumask.h:12,
>                  from ./include/linux/smp.h:13,
>                  from ./include/linux/lockdep.h:14,
>                  from ./include/linux/spinlock.h:63,
>                  from ./include/linux/wait.h:9,
>                  from ./include/linux/wait_bit.h:8,
>                  from ./include/linux/fs.h:6,
>                  from kernel/auditsc.c:37:
> In function ‘sized_strscpy’,
>     inlined from ‘__audit_ptrace’ at kernel/auditsc.c:2732:2:
> ./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
>   293 |   __write_overflow();
>       |   ^~~~~~~~~~~~~~~~~~
> In function ‘sized_strscpy’,
>     inlined from ‘audit_signal_info_syscall’ at kernel/auditsc.c:2759:3:
> ./include/linux/fortify-string.h:293:3: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object (1st parameter)
>   293 |   __write_overflow();
>
> i see commit d9381508ea2b590aff46d28d432d20bfef1ba64c merged,
> but it's a workaround, so how about my thoughts below?

Before we get too far into a workaround, can you confirm that you are
not seeing the error above with what is currently in Linus' tree, e.g.
commit d9381508ea2b ("audit: workaround a GCC bug triggered by task
comm changes")?  If that has resolved your problem, I think we can
leave things as they are for now, we have one workaround in Linus'
tree that should mask the compiler bug and I'm not excited about
adding another.

Like Casey, I'm not excited about adding a useless field to the lsm_prop struct.

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index cbdba435b798..f502deecb142 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -164,6 +164,7 @@  struct lsm_prop {
 	struct lsm_prop_smack smack;
 	struct lsm_prop_apparmor apparmor;
 	struct lsm_prop_bpf bpf;
+	u8 reserved;
 };
 
 extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];