2021-07-15 13:33:59

by Tianjia Zhang

Subject: [PATCH] Smack: Fix wrong semantics in smk_access_entry()

In the smk_access_entry() function, if no matching rule is found
in the rust_list, a negative error code will be used to perform bit
operations with the MAY_ enumeration value. This is semantically
wrong. This patch fixes this issue.

Signed-off-by: Tianjia Zhang <[email protected]>
---
security/smack/smack_access.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 1f391f6a3d47..d2186e2757be 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -81,23 +81,22 @@ int log_policy = SMACK_AUDIT_DENIED;
int smk_access_entry(char *subject_label, char *object_label,
struct list_head *rule_list)
{
- int may = -ENOENT;
struct smack_rule *srp;

list_for_each_entry_rcu(srp, rule_list, list) {
if (srp->smk_object->smk_known == object_label &&
srp->smk_subject->smk_known == subject_label) {
- may = srp->smk_access;
- break;
+ int may = srp->smk_access;
+ /*
+ * MAY_WRITE implies MAY_LOCK.
+ */
+ if ((may & MAY_WRITE) == MAY_WRITE)
+ may |= MAY_LOCK;
+ return may;
}
}

- /*
- * MAY_WRITE implies MAY_LOCK.
- */
- if ((may & MAY_WRITE) == MAY_WRITE)
- may |= MAY_LOCK;
- return may;
+ return -ENOENT;
}

/**
--
2.19.1.3.ge56e4f7

2021-07-15 16:49:52

by Casey Schaufler

Subject: Re: [PATCH] Smack: Fix wrong semantics in smk_access_entry()

On 7/15/2021 2:17 AM, Tianjia Zhang wrote:
> In the smk_access_entry() function, if no matching rule is found
> in the rust_list, a negative error code will be used to perform bit
> operations with the MAY_ enumeration value. This is semantically
> wrong. This patch fixes this issue.

Indeed, the code as written is functioning correctly by
sheer luck. I will take this patch. Thank you.

>
> Signed-off-by: Tianjia Zhang <[email protected]>
> ---
> security/smack/smack_access.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index 1f391f6a3d47..d2186e2757be 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -81,23 +81,22 @@ int log_policy = SMACK_AUDIT_DENIED;
> int smk_access_entry(char *subject_label, char *object_label,
> struct list_head *rule_list)
> {
> - int may = -ENOENT;
> struct smack_rule *srp;
>
> list_for_each_entry_rcu(srp, rule_list, list) {
> if (srp->smk_object->smk_known == object_label &&
> srp->smk_subject->smk_known == subject_label) {
> - may = srp->smk_access;
> - break;
> + int may = srp->smk_access;
> + /*
> + * MAY_WRITE implies MAY_LOCK.
> + */
> + if ((may & MAY_WRITE) == MAY_WRITE)
> + may |= MAY_LOCK;
> + return may;
> }
> }
>
> - /*
> - * MAY_WRITE implies MAY_LOCK.
> - */
> - if ((may & MAY_WRITE) == MAY_WRITE)
> - may |= MAY_LOCK;
> - return may;
> + return -ENOENT;
> }
>
> /**

2021-07-20 16:38:28

by Casey Schaufler

Subject: Re: [PATCH] Smack: Fix wrong semantics in smk_access_entry()

On 7/15/2021 8:15 AM, Casey Schaufler wrote:
> On 7/15/2021 2:17 AM, Tianjia Zhang wrote:
>> In the smk_access_entry() function, if no matching rule is found
>> in the rust_list, a negative error code will be used to perform bit
>> operations with the MAY_ enumeration value. This is semantically
>> wrong. This patch fixes this issue.
> Indeed, the code as written is functioning correctly by
> sheer luck. I will take this patch. Thank you.
>
>> Signed-off-by: Tianjia Zhang <[email protected]>

Added to the Smack next branch.

>> ---
>> security/smack/smack_access.c | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
>> index 1f391f6a3d47..d2186e2757be 100644
>> --- a/security/smack/smack_access.c
>> +++ b/security/smack/smack_access.c
>> @@ -81,23 +81,22 @@ int log_policy = SMACK_AUDIT_DENIED;
>> int smk_access_entry(char *subject_label, char *object_label,
>> struct list_head *rule_list)
>> {
>> - int may = -ENOENT;
>> struct smack_rule *srp;
>>
>> list_for_each_entry_rcu(srp, rule_list, list) {
>> if (srp->smk_object->smk_known == object_label &&
>> srp->smk_subject->smk_known == subject_label) {
>> - may = srp->smk_access;
>> - break;
>> + int may = srp->smk_access;
>> + /*
>> + * MAY_WRITE implies MAY_LOCK.
>> + */
>> + if ((may & MAY_WRITE) == MAY_WRITE)
>> + may |= MAY_LOCK;
>> + return may;
>> }
>> }
>>
>> - /*
>> - * MAY_WRITE implies MAY_LOCK.
>> - */
>> - if ((may & MAY_WRITE) == MAY_WRITE)
>> - may |= MAY_LOCK;
>> - return may;
>> + return -ENOENT;
>> }
>>
>> /**

2021-07-21 03:16:09

by Tianjia Zhang

Subject: Re: [PATCH] Smack: Fix wrong semantics in smk_access_entry()

On 7/21/21 12:32 AM, Casey Schaufler wrote:
> On 7/15/2021 8:15 AM, Casey Schaufler wrote:
>> On 7/15/2021 2:17 AM, Tianjia Zhang wrote:
>>> In the smk_access_entry() function, if no matching rule is found
>>> in the rust_list, a negative error code will be used to perform bit
>>> operations with the MAY_ enumeration value. This is semantically
>>> wrong. This patch fixes this issue.
>> Indeed, the code as written is functioning correctly by
>> sheer luck. I will take this patch. Thank you.
>>
>>> Signed-off-by: Tianjia Zhang <[email protected]>
>
> Added to the Smack next branch.
>

Great, Thanks.

Tianjia