[PATCH bpf-next v2 2/7] bpf, lsm: Add return value range description for lsm hook

[Xu Kuohai]

On 4/9/2024 6:15 AM, KP Singh wrote:
> On Mon, Apr 8, 2024 at 7:09 PM KP Singh <kpsingh at kernel.org> wrote:
>>
>>
>>
>>> On 25 Mar 2024, at 10:56, Xu Kuohai <xukuohai at huaweicloud.com> wrote:
>>>
>>> From: Xu Kuohai <xukuohai at huawei.com>
>>>
>>> Add return value descriptions for lsm hook.
>>>
>>> Two integer ranges are added:
>>>
>>> 1. ERRNO: Integer between -MAX_ERRNO and 0, including -MAX_ERRNO and 0.
> 
> I also don't really like these special macros that imply a range. Why
> not do something like?
> 
>    LSM_RET_INT(default, min, max)
>

This also looks good to me, will update in next version.

> You seemed to have missed the values returned by these hooks:
> 
> security_inode_need_killpriv
> security_inode_getsecurity
> security_inode_listsecurity
> security_inode_copy_up_xattr
> security_task_prctl
> 
> security_getprocattr
> securitty_setprocattr
> ^^these two we should just disable in BPF LSM
> 
> security_ismaclabel
> ^^probably even this
>

Good catch! I've fixed the return value ranges based on the return value
description in file security/security.c.

Here are the fixed functions and their return ranges.

security_inode_need_killpriv			[-MAX_ERRNO, INT_MAX]
security_inode_getsecurity			[-MAX_ERRNO, INT_MAX]
security_inode_listsecurity			[-MAX_ERRNO, INT_MAX]
security_inode_copy_up_xattr			[-MAX_ERRNO, 1]
security_getselfattr				[-MAX_ERRNO, INT_MAX]
security_getprocattr				[-MAX_ERRNO, INT_MAX]
security_setprocattr				[-MAX_ERRNO, INT_MAX]
security_task_prctl				[-MAX_ERRNO, INT_MAX]
security_ismaclabel				[0, 1]
security_xfrm_state_pol_flow_match		[INT_MIN, INT_MAX]
security_key_getsecurity			[-MAX_ERRNO, INT_MAX]
security_audit_rule_known			[0, 1]
security_audit_rule_match			[-MAX_ERRNO, 1]

> There seem to be only a handful of these. Can we just manage it with a
> BTF set on the BPF side?
> 

I'll add a disabled bpf lsm hook list on the bpf side.

> - KP
>>> 2. ANY: Any integer
>>
>>
>> I think you should merge this patch and the first patch. It's not clear that the first value in this macro is actually used as the default value until one reads the code. I think you also need to make it clear that there is no logical change on the LSM side in the this patch.

OK.

>>
>> - KP