[PATCH 1/1] selinux: Fix potential counting error in avc_add_xperms_decision()

[PATCH 1/1] selinux: Fix potential counting error in avc_add_xperms_decision()

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

The count increases only when a node is successfully added to
the linked list.

Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 security/selinux/avc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 32eb67fb3e42c0f..7087cd2b802d8d8 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -330,12 +330,12 @@ static int avc_add_xperms_decision(struct avc_node *node,
 {
 	struct avc_xperms_decision_node *dest_xpd;
 
-	node->ae.xp_node->xp.len++;
 	dest_xpd = avc_xperms_decision_alloc(src->used);
 	if (!dest_xpd)
 		return -ENOMEM;
 	avc_copy_xperms_decision(&dest_xpd->xpd, src);
 	list_add(&dest_xpd->xpd_list, &node->ae.xp_node->xpd_head);
+	node->ae.xp_node->xp.len++;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/1] selinux: Fix potential counting error in avc_add_xperms_decision()

[Stephen Smalley]

On Tue, Aug 6, 2024 at 2:51 AM <thunder.leizhen@huaweicloud.com> wrote:
>
> From: Zhen Lei <thunder.leizhen@huawei.com>
>
> The count increases only when a node is successfully added to
> the linked list.
>
> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

This looks correct to me but I also notice that the caller is not
checking or handling the return code for the -ENOMEM situation.
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
>  security/selinux/avc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 32eb67fb3e42c0f..7087cd2b802d8d8 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -330,12 +330,12 @@ static int avc_add_xperms_decision(struct avc_node *node,
>  {
>         struct avc_xperms_decision_node *dest_xpd;
>
> -       node->ae.xp_node->xp.len++;
>         dest_xpd = avc_xperms_decision_alloc(src->used);
>         if (!dest_xpd)
>                 return -ENOMEM;
>         avc_copy_xperms_decision(&dest_xpd->xpd, src);
>         list_add(&dest_xpd->xpd_list, &node->ae.xp_node->xpd_head);
> +       node->ae.xp_node->xp.len++;
>         return 0;
>  }
>
> --
> 2.34.1
>

Re: [PATCH 1/1] selinux: Fix potential counting error in avc_add_xperms_decision()

[Paul Moore]

On Tue, Aug 6, 2024 at 9:26 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Aug 6, 2024 at 2:51 AM <thunder.leizhen@huaweicloud.com> wrote:
> > From: Zhen Lei <thunder.leizhen@huawei.com>
> >
> > The count increases only when a node is successfully added to
> > the linked list.
> >
> > Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>
> This looks correct to me ...

It looks good to me too, unless I hear any objections I'm going to
merge this into selinux/stable-6.11 and send it up to Linux during the
v6.11-rcX cycle.

> ... but I also notice that the caller is not
> checking or handling the return code for the -ENOMEM situation.

Good catch.  We should also fix this, ideally in the same PR where we
send the count/len fix.

Zhen Lei, would you mind working on a separate fix for checking the
error code in the caller?

-- 
paul-moore.com

Re: [PATCH 1/1] selinux: Fix potential counting error in avc_add_xperms_decision()

[Leizhen (ThunderTown)]

On 2024/8/7 5:55, Paul Moore wrote:
> On Tue, Aug 6, 2024 at 9:26 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Tue, Aug 6, 2024 at 2:51 AM <thunder.leizhen@huaweicloud.com> wrote:
>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> The count increases only when a node is successfully added to
>>> the linked list.
>>>
>>> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> This looks correct to me ...
> 
> It looks good to me too, unless I hear any objections I'm going to
> merge this into selinux/stable-6.11 and send it up to Linux during the
> v6.11-rcX cycle.
> 
>> ... but I also notice that the caller is not
>> checking or handling the return code for the -ENOMEM situation.
> 
> Good catch.  We should also fix this, ideally in the same PR where we
> send the count/len fix.
> 
> Zhen Lei, would you mind working on a separate fix for checking the
> error code in the caller?

Yeah, I'd love to.

> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] selinux: Fix potential counting error in avc_add_xperms_decision()

[Markus Elfring]

> The count increases only when a node is successfully added to
> the linked list.

1. Please improve such a change description with an imperative wording.
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc2#n94

2. How do you think about to omit the word “potential” from the summary phrase?


Regards,
Markus

Re: [PATCH] selinux: Fix potential counting error in avc_add_xperms_decision()

[Leizhen (ThunderTown)]

On 2024/8/7 0:30, Markus Elfring wrote:
>> The count increases only when a node is successfully added to
>> the linked list.
> 
> 1. Please improve such a change description with an imperative wording.
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc2#n94
Ok, I'll try to improve it.

> 
> 2. How do you think about to omit the word “potential” from the summary phrase?
I added "potential" because it's unlikely that the memory request will fail. Maybe "possible" would
be better.


> 
> 
> Regards,
> Markus
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] selinux: Fix potential counting error in avc_add_xperms_decision()

[Leizhen (ThunderTown)]

On 2024/8/7 17:16, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/8/7 0:30, Markus Elfring wrote:
>>> The count increases only when a node is successfully added to
>>> the linked list.
>>
>> 1. Please improve such a change description with an imperative wording.
>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc2#n94
> Ok, I'll try to improve it.
I see this patch has been merged into selinux/stable-6.11. So I decided not to
change it, and I re-examined it, and it seems that there is no problem you
mentioned.

> 
>>
>> 2. How do you think about to omit the word “potential” from the summary phrase?
> I added "potential" because it's unlikely that the memory request will fail. Maybe "possible" would
> be better.
> 
> 
>>
>>
>> Regards,
>> Markus
>> .
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] selinux: Fix potential counting error in avc_add_xperms_decision()

[Markus Elfring]

>>>> The count increases only when a node is successfully added to
>>>> the linked list.
>>>
>>> 1. Please improve such a change description with an imperative wording.
>>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc2#n94
>> Ok, I'll try to improve it.
> I see this patch has been merged into selinux/stable-6.11.

Interesting …


> So I decided not to change it, and I re-examined it,

Further collateral evolution can become more helpful,
can't it?


> and it seems that there is no problem you mentioned.

There are obviously different preferences involved for patch review processes.

Regards,
Markus

Re: [PATCH] selinux: Fix potential counting error in avc_add_xperms_decision()

[Leizhen (ThunderTown)]

On 2024/8/7 20:06, Markus Elfring wrote:
>>>>> The count increases only when a node is successfully added to
>>>>> the linked list.
>>>>
>>>> 1. Please improve such a change description with an imperative wording.
>>>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc2#n94
>>> Ok, I'll try to improve it.
>> I see this patch has been merged into selinux/stable-6.11.
> 
> Interesting …
It's not surprising. Because maintainers deal with programmers in many countries,
they will find over time that some programmers write descriptions that are not
pleasing to the eye, not his intention, but poor English. So step back, as long
as the patch is correct and clearly describes why it's done, it's enough. It's
not a bad thing to be more inclusive of others.

> 
> 
>> So I decided not to change it, and I re-examined it,
> 
> Further collateral evolution can become more helpful,
> can't it?
> 
> 
>> and it seems that there is no problem you mentioned.
> 
> There are obviously different preferences involved for patch review processes.
> 
> Regards,
> Markus
> .
> 

-- 
Regards,
  Zhen Lei