| Message ID | 20240711025852.916781-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [-next] selinux: refactor code to return the correct errno | expand |
On Jul 10, 2024 Gaosheng Cui <cuigaosheng1@huawei.com> wrote: > > Refactor the code in sel_netif_sid_slow to get the correct errno > when an error occurs. > > Add some similar modifications to selinux_netlbl_sock_genattr and > sel_netport_sid_slow. > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > security/selinux/netif.c | 16 ++++++++++------ > security/selinux/netlabel.c | 16 ++++++++-------- > security/selinux/netport.c | 12 +++++++----- > 3 files changed, 25 insertions(+), 19 deletions(-) > > diff --git a/security/selinux/netif.c b/security/selinux/netif.c > index 43a0d3594b72..6d8544d8c63c 100644 > --- a/security/selinux/netif.c > +++ b/security/selinux/netif.c > @@ -156,14 +156,18 @@ static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid) > ret = security_netif_sid(dev->name, sid); > if (ret != 0) > goto out; > + > new = kzalloc(sizeof(*new), GFP_ATOMIC); > - if (new) { > - new->nsec.ns = ns; > - new->nsec.ifindex = ifindex; > - new->nsec.sid = *sid; > - if (sel_netif_insert(new)) > - kfree(new); > + if (!new) { > + ret = -ENOMEM; > + goto out; > } > + new->nsec.ns = ns; > + new->nsec.ifindex = ifindex; > + new->nsec.sid = *sid; > + ret = sel_netif_insert(new); > + if (ret) > + kfree(new); The case where we fail add the new netif to the cache should not return an error as we were able to successfully lookup the SELinux SID for the netif and return it to the caller. Yes, we were not able to add it to the cache, but that doesn't mean we should fail the operation by returning an error code. > out: > spin_unlock_bh(&sel_netif_lock); > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > index 55885634e880..40b5dcbd97d4 100644 > --- a/security/selinux/netlabel.c > +++ b/security/selinux/netlabel.c > @@ -76,11 +76,12 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_genattr(struct sock *sk) > > secattr = netlbl_secattr_alloc(GFP_ATOMIC); > if (secattr == NULL) > - return NULL; > + return ERR_PTR(-ENOMEM); > + > rc = security_netlbl_sid_to_secattr(sksec->sid, secattr); > if (rc != 0) { > netlbl_secattr_free(secattr); > - return NULL; > + return ERR_PTR(rc); > } > sksec->nlbl_secattr = secattr; You need to update the function's comment header to indicate that it returns error codes encoded with ERR_PTR() on failure. > diff --git a/security/selinux/netport.c b/security/selinux/netport.c > index 2e22ad9c2bd0..a75a479515fb 100644 > --- a/security/selinux/netport.c > +++ b/security/selinux/netport.c > @@ -152,12 +152,14 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > if (ret != 0) > goto out; > new = kzalloc(sizeof(*new), GFP_ATOMIC); > - if (new) { > - new->psec.port = pnum; > - new->psec.protocol = protocol; > - new->psec.sid = *sid; > - sel_netport_insert(new); > + if (!new) { > + ret = -ENOMEM; > + goto out; > } > + new->psec.port = pnum; > + new->psec.protocol = protocol; > + new->psec.sid = *sid; > + sel_netport_insert(new); Same logic as sel_netif_sid_slow(). -- paul-moore.com
Thanks for your work. I've removed the modifications to netif and netport, and update the selinux_netlbl_sock_genattr's comment header. On 2024/7/12 5:19, Paul Moore wrote: > On Jul 10, 2024 Gaosheng Cui <cuigaosheng1@huawei.com> wrote: >> Refactor the code in sel_netif_sid_slow to get the correct errno >> when an error occurs. >> >> Add some similar modifications to selinux_netlbl_sock_genattr and >> sel_netport_sid_slow. >> >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >> --- >> security/selinux/netif.c | 16 ++++++++++------ >> security/selinux/netlabel.c | 16 ++++++++-------- >> security/selinux/netport.c | 12 +++++++----- >> 3 files changed, 25 insertions(+), 19 deletions(-) >> >> diff --git a/security/selinux/netif.c b/security/selinux/netif.c >> index 43a0d3594b72..6d8544d8c63c 100644 >> --- a/security/selinux/netif.c >> +++ b/security/selinux/netif.c >> @@ -156,14 +156,18 @@ static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid) >> ret = security_netif_sid(dev->name, sid); >> if (ret != 0) >> goto out; >> + >> new = kzalloc(sizeof(*new), GFP_ATOMIC); >> - if (new) { >> - new->nsec.ns = ns; >> - new->nsec.ifindex = ifindex; >> - new->nsec.sid = *sid; >> - if (sel_netif_insert(new)) >> - kfree(new); >> + if (!new) { >> + ret = -ENOMEM; >> + goto out; >> } >> + new->nsec.ns = ns; >> + new->nsec.ifindex = ifindex; >> + new->nsec.sid = *sid; >> + ret = sel_netif_insert(new); >> + if (ret) >> + kfree(new); > The case where we fail add the new netif to the cache should not return > an error as we were able to successfully lookup the SELinux SID for the > netif and return it to the caller. Yes, we were not able to add it to > the cache, but that doesn't mean we should fail the operation by > returning an error code. > >> out: >> spin_unlock_bh(&sel_netif_lock); >> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c >> index 55885634e880..40b5dcbd97d4 100644 >> --- a/security/selinux/netlabel.c >> +++ b/security/selinux/netlabel.c >> @@ -76,11 +76,12 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_genattr(struct sock *sk) >> >> secattr = netlbl_secattr_alloc(GFP_ATOMIC); >> if (secattr == NULL) >> - return NULL; >> + return ERR_PTR(-ENOMEM); >> + >> rc = security_netlbl_sid_to_secattr(sksec->sid, secattr); >> if (rc != 0) { >> netlbl_secattr_free(secattr); >> - return NULL; >> + return ERR_PTR(rc); >> } >> sksec->nlbl_secattr = secattr; > You need to update the function's comment header to indicate that it > returns error codes encoded with ERR_PTR() on failure. > >> diff --git a/security/selinux/netport.c b/security/selinux/netport.c >> index 2e22ad9c2bd0..a75a479515fb 100644 >> --- a/security/selinux/netport.c >> +++ b/security/selinux/netport.c >> @@ -152,12 +152,14 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) >> if (ret != 0) >> goto out; >> new = kzalloc(sizeof(*new), GFP_ATOMIC); >> - if (new) { >> - new->psec.port = pnum; >> - new->psec.protocol = protocol; >> - new->psec.sid = *sid; >> - sel_netport_insert(new); >> + if (!new) { >> + ret = -ENOMEM; >> + goto out; >> } >> + new->psec.port = pnum; >> + new->psec.protocol = protocol; >> + new->psec.sid = *sid; >> + sel_netport_insert(new); > Same logic as sel_netif_sid_slow(). > > -- > paul-moore.com > .
diff --git a/security/selinux/netif.c b/security/selinux/netif.c index 43a0d3594b72..6d8544d8c63c 100644 --- a/security/selinux/netif.c +++ b/security/selinux/netif.c @@ -156,14 +156,18 @@ static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid) ret = security_netif_sid(dev->name, sid); if (ret != 0) goto out; + new = kzalloc(sizeof(*new), GFP_ATOMIC); - if (new) { - new->nsec.ns = ns; - new->nsec.ifindex = ifindex; - new->nsec.sid = *sid; - if (sel_netif_insert(new)) - kfree(new); + if (!new) { + ret = -ENOMEM; + goto out; } + new->nsec.ns = ns; + new->nsec.ifindex = ifindex; + new->nsec.sid = *sid; + ret = sel_netif_insert(new); + if (ret) + kfree(new); out: spin_unlock_bh(&sel_netif_lock); diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 55885634e880..40b5dcbd97d4 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -76,11 +76,12 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_genattr(struct sock *sk) secattr = netlbl_secattr_alloc(GFP_ATOMIC); if (secattr == NULL) - return NULL; + return ERR_PTR(-ENOMEM); + rc = security_netlbl_sid_to_secattr(sksec->sid, secattr); if (rc != 0) { netlbl_secattr_free(secattr); - return NULL; + return ERR_PTR(rc); } sksec->nlbl_secattr = secattr; @@ -400,8 +401,8 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) return 0; secattr = selinux_netlbl_sock_genattr(sk); - if (secattr == NULL) - return -ENOMEM; + if (IS_ERR(secattr)) + return PTR_ERR(secattr); /* On socket creation, replacement of IP options is safe even if * the caller does not hold the socket lock. */ @@ -561,10 +562,9 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk, return rc; } secattr = selinux_netlbl_sock_genattr(sk); - if (secattr == NULL) { - rc = -ENOMEM; - return rc; - } + if (IS_ERR(secattr)) + return PTR_ERR(secattr); + rc = netlbl_conn_setattr(sk, addr, secattr); if (rc == 0) sksec->nlbl_state = NLBL_CONNLABELED; diff --git a/security/selinux/netport.c b/security/selinux/netport.c index 2e22ad9c2bd0..a75a479515fb 100644 --- a/security/selinux/netport.c +++ b/security/selinux/netport.c @@ -152,12 +152,14 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) if (ret != 0) goto out; new = kzalloc(sizeof(*new), GFP_ATOMIC); - if (new) { - new->psec.port = pnum; - new->psec.protocol = protocol; - new->psec.sid = *sid; - sel_netport_insert(new); + if (!new) { + ret = -ENOMEM; + goto out; } + new->psec.port = pnum; + new->psec.protocol = protocol; + new->psec.sid = *sid; + sel_netport_insert(new); out: spin_unlock_bh(&sel_netport_lock);
Refactor the code in sel_netif_sid_slow to get the correct errno when an error occurs. Add some similar modifications to selinux_netlbl_sock_genattr and sel_netport_sid_slow. Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> --- security/selinux/netif.c | 16 ++++++++++------ security/selinux/netlabel.c | 16 ++++++++-------- security/selinux/netport.c | 12 +++++++----- 3 files changed, 25 insertions(+), 19 deletions(-)