| Message ID | 20240806065113.1317-1-thunder.leizhen@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/1] selinux: Fix potential counting error in avc_add_xperms_decision() | expand |
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 >
> 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
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?
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. >
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 > . >
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 >> . >> >
>>>> 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
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 > . >
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; }