* [PATCH 2/5] selinux: use correct type for context length @ 2022-02-17 14:21 Christian Göttsche 2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche ` (4 more replies) 0 siblings, 5 replies; 24+ messages in thread From: Christian Göttsche @ 2022-02-17 14:21 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Austin Kim, Kees Cook, Yang Li, Casey Schaufler, linux-kernel, llvm security_sid_to_context() expects a pointer to an u32 as the address where to store the length of the computed context. Reported by sparse: security/selinux/xfrm.c:359:39: warning: incorrect type in argument 4 (different signedness) security/selinux/xfrm.c:359:39: expected unsigned int [usertype] *scontext_len security/selinux/xfrm.c:359:39: got int * Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/xfrm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index 90697317895f..c576832febc6 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -347,7 +347,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x, int rc; struct xfrm_sec_ctx *ctx; char *ctx_str = NULL; - int str_len; + u32 str_len; if (!polsec) return 0; -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/5] selinux: use consistent pointer types for boolean arrays 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche @ 2022-02-17 14:21 ` Christian Göttsche 2022-02-18 16:01 ` Paul Moore 2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche ` (3 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: Christian Göttsche @ 2022-02-17 14:21 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, James Morris, Austin Kim, Casey Schaufler, Jiapeng Chong, Yang Li, linux-kernel, llvm Use a consistent type of unsigned int* for boolean arrays, instead of using implicit casts to and from int*. Reported by sparse: security/selinux/selinuxfs.c:1481:30: warning: incorrect type in assignment (different signedness) security/selinux/selinuxfs.c:1481:30: expected unsigned int * security/selinux/selinuxfs.c:1481:30: got int *[addressable] values security/selinux/selinuxfs.c:1398:48: warning: incorrect type in argument 3 (different signedness) security/selinux/selinuxfs.c:1398:48: expected int *values security/selinux/selinuxfs.c:1398:48: got unsigned int *bool_pending_values Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- A more invasive change would be to change all boolean arrays to bool*. --- security/selinux/include/conditional.h | 4 ++-- security/selinux/selinuxfs.c | 2 +- security/selinux/ss/services.c | 9 +++++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h index b09343346e3f..9e65aa409318 100644 --- a/security/selinux/include/conditional.h +++ b/security/selinux/include/conditional.h @@ -14,9 +14,9 @@ #include "security.h" int security_get_bools(struct selinux_policy *policy, - u32 *len, char ***names, int **values); + u32 *len, char ***names, unsigned int **values); -int security_set_bools(struct selinux_state *state, u32 len, int *values); +int security_set_bools(struct selinux_state *state, u32 len, unsigned int *values); int security_get_bool_value(struct selinux_state *state, u32 index); diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index f2f6203e0fff..5216a321bbb0 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1428,7 +1428,7 @@ static int sel_make_bools(struct selinux_policy *newpolicy, struct dentry *bool_ struct inode_security_struct *isec; char **names = NULL, *page; u32 i, num; - int *values = NULL; + unsigned int *values = NULL; u32 sid; ret = -ENOMEM; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 6901dc07680d..7865926962ab 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -3023,7 +3023,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) } int security_get_bools(struct selinux_policy *policy, - u32 *len, char ***names, int **values) + u32 *len, char ***names, unsigned int **values) { struct policydb *policydb; u32 i; @@ -3045,7 +3045,7 @@ int security_get_bools(struct selinux_policy *policy, goto err; rc = -ENOMEM; - *values = kcalloc(*len, sizeof(int), GFP_ATOMIC); + *values = kcalloc(*len, sizeof(unsigned int), GFP_ATOMIC); if (!*values) goto err; @@ -3075,7 +3075,7 @@ int security_get_bools(struct selinux_policy *policy, } -int security_set_bools(struct selinux_state *state, u32 len, int *values) +int security_set_bools(struct selinux_state *state, u32 len, unsigned int *values) { struct selinux_policy *newpolicy, *oldpolicy; int rc; @@ -3175,7 +3175,8 @@ int security_get_bool_value(struct selinux_state *state, static int security_preserve_bools(struct selinux_policy *oldpolicy, struct selinux_policy *newpolicy) { - int rc, *bvalues = NULL; + int rc; + unsigned int *bvalues = NULL; char **bnames = NULL; struct cond_bool_datum *booldatum; u32 i, nbools = 0; -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] selinux: use consistent pointer types for boolean arrays 2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche @ 2022-02-18 16:01 ` Paul Moore 2022-03-08 15:57 ` Christian Göttsche 0 siblings, 1 reply; 24+ messages in thread From: Paul Moore @ 2022-02-18 16:01 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, James Morris, Austin Kim, Casey Schaufler, Jiapeng Chong, Yang Li, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Use a consistent type of unsigned int* for boolean arrays, instead of > using implicit casts to and from int*. > > Reported by sparse: > > security/selinux/selinuxfs.c:1481:30: warning: incorrect type in assignment (different signedness) > security/selinux/selinuxfs.c:1481:30: expected unsigned int * > security/selinux/selinuxfs.c:1481:30: got int *[addressable] values > security/selinux/selinuxfs.c:1398:48: warning: incorrect type in argument 3 (different signedness) > security/selinux/selinuxfs.c:1398:48: expected int *values > security/selinux/selinuxfs.c:1398:48: got unsigned int *bool_pending_values > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > A more invasive change would be to change all boolean arrays to bool*. I think that might be a worthwhile change, although that can happen at a later date. A quick general comment: please try to stick to 80-char long lines. I realize Linus/checkpatch.pl has started to allow longer lines but I would still like SELinux to try and keep to 80-chars or under. > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 6901dc07680d..7865926962ab 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -3175,7 +3175,8 @@ int security_get_bool_value(struct selinux_state *state, > static int security_preserve_bools(struct selinux_policy *oldpolicy, > struct selinux_policy *newpolicy) > { > - int rc, *bvalues = NULL; > + int rc; > + unsigned int *bvalues = NULL; Doesn't this cause a type mismatch (unsigned int vs int) when an entry from bvalues[] is assigned to cond_bool_datum::state later in the security_preserve_bools() function? -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] selinux: use consistent pointer types for boolean arrays 2022-02-18 16:01 ` Paul Moore @ 2022-03-08 15:57 ` Christian Göttsche 0 siblings, 0 replies; 24+ messages in thread From: Christian Göttsche @ 2022-03-08 15:57 UTC (permalink / raw) To: Paul Moore Cc: SElinux list, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, James Morris, Austin Kim, Casey Schaufler, Jiapeng Chong, Yang Li, Linux kernel mailing list, llvm On Fri, 18 Feb 2022 at 17:01, Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Use a consistent type of unsigned int* for boolean arrays, instead of > > using implicit casts to and from int*. > > > > Reported by sparse: > > > > security/selinux/selinuxfs.c:1481:30: warning: incorrect type in assignment (different signedness) > > security/selinux/selinuxfs.c:1481:30: expected unsigned int * > > security/selinux/selinuxfs.c:1481:30: got int *[addressable] values > > security/selinux/selinuxfs.c:1398:48: warning: incorrect type in argument 3 (different signedness) > > security/selinux/selinuxfs.c:1398:48: expected int *values > > security/selinux/selinuxfs.c:1398:48: got unsigned int *bool_pending_values > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > --- > > A more invasive change would be to change all boolean arrays to bool*. > > I think that might be a worthwhile change, although that can happen at > a later date. > > A quick general comment: please try to stick to 80-char long lines. I > realize Linus/checkpatch.pl has started to allow longer lines but I > would still like SELinux to try and keep to 80-chars or under. > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 6901dc07680d..7865926962ab 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -3175,7 +3175,8 @@ int security_get_bool_value(struct selinux_state *state, > > static int security_preserve_bools(struct selinux_policy *oldpolicy, > > struct selinux_policy *newpolicy) > > { > > - int rc, *bvalues = NULL; > > + int rc; > > + unsigned int *bvalues = NULL; > > Doesn't this cause a type mismatch (unsigned int vs int) when an entry > from bvalues[] is assigned to cond_bool_datum::state later in the > security_preserve_bools() function? Yes, but those variables *should* only hold the values 0 or 1. But probably it's better to re-spin for 5.19 with all arrays and cond_bool_datum::state converted to literal bool type. > > -- > paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/5] selinux: declare data arrays const 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche 2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche @ 2022-02-17 14:21 ` Christian Göttsche 2022-02-18 16:13 ` Paul Moore 2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche ` (2 subsequent siblings) 4 siblings, 2 replies; 24+ messages in thread From: Christian Göttsche @ 2022-02-17 14:21 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Jeremy Kerr, David S. Miller, Lakshmi Ramasubramanian, Yang Li, Austin Kim, linux-kernel, llvm The arrays for the policy capability names, the initial sid identifiers and the class and permission names are not changed at runtime. Declare them const to avoid accidental modification. The build time script genheaders needs to be exempted, since it converts the entries to uppercase. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- scripts/selinux/genheaders/genheaders.c | 2 ++ scripts/selinux/mdp/mdp.c | 4 ++-- security/selinux/avc.c | 2 +- security/selinux/include/avc_ss.h | 2 +- security/selinux/include/classmap.h | 8 +++++++- security/selinux/include/initial_sid_to_string.h | 9 ++++++++- security/selinux/include/policycap.h | 2 +- security/selinux/include/policycap_names.h | 2 +- security/selinux/ss/services.c | 4 ++-- 9 files changed, 25 insertions(+), 10 deletions(-) diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index f355b3e0e968..5f7c0b7d9260 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -15,6 +15,8 @@ struct security_class_mapping { const char *perms[sizeof(unsigned) * 8 + 1]; }; +/* Allow to convert entries in mappings to uppercase */ +#define __SELINUX_GENHEADERS__ #include "classmap.h" #include "initial_sid_to_string.h" diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c index 105c1c31a316..1415604c3d24 100644 --- a/scripts/selinux/mdp/mdp.c +++ b/scripts/selinux/mdp/mdp.c @@ -82,7 +82,7 @@ int main(int argc, char *argv[]) /* print out the class permissions */ for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "class %s\n", map->name); fprintf(fout, "{\n"); for (j = 0; map->perms[j]; j++) @@ -103,7 +103,7 @@ int main(int argc, char *argv[]) #define SYSTEMLOW "s0" #define SYSTEMHIGH "s1:c0.c1" for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "mlsconstrain %s {\n", map->name); for (j = 0; map->perms[j]; j++) diff --git a/security/selinux/avc.c b/security/selinux/avc.c index abcd9740d10f..020985a53d8f 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -668,7 +668,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) struct common_audit_data *ad = a; struct selinux_audit_data *sad = ad->selinux_audit_data; u32 av = sad->audited; - const char **perms; + const char *const *perms; int i, perm; audit_log_format(ab, "avc: %s ", sad->denied ? "denied" : "granted"); diff --git a/security/selinux/include/avc_ss.h b/security/selinux/include/avc_ss.h index 88c384c5c09e..b38974e22d81 100644 --- a/security/selinux/include/avc_ss.h +++ b/security/selinux/include/avc_ss.h @@ -18,7 +18,7 @@ struct security_class_mapping { const char *perms[sizeof(u32) * 8 + 1]; }; -extern struct security_class_mapping secclass_map[]; +extern const struct security_class_mapping secclass_map[]; #endif /* _SELINUX_AVC_SS_H_ */ diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 35aac62a662e..07ade4af85ff 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -2,6 +2,12 @@ #include <linux/capability.h> #include <linux/socket.h> +#ifdef __SELINUX_GENHEADERS__ +# define const_qual +#else +# define const_qual const +#endif + #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" @@ -38,7 +44,7 @@ * Note: The name for any socket class should be suffixed by "socket", * and doesn't contain more than one substr of "socket". */ -struct security_class_mapping secclass_map[] = { +const_qual struct security_class_mapping secclass_map[] = { { "security", { "compute_av", "compute_create", "compute_member", "check_context", "load_policy", "compute_relabel", diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 5d332aeb8b6c..915283cd89bd 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,5 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0 */ -static const char *initial_sid_to_string[] = + +#ifdef __SELINUX_GENHEADERS__ +# define const_qual +#else +# define const_qual const +#endif + +static const char *const_qual initial_sid_to_string[] = { NULL, "kernel", diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index 2ec038efbb03..3207a4e8c899 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -15,6 +15,6 @@ enum { }; #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) -extern const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; +extern const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; #endif /* _SELINUX_POLICYCAP_H_ */ diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index b89289f092c9..51da36e37d21 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -5,7 +5,7 @@ #include "policycap.h" /* Policy capability names */ -const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { +const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { "network_peer_controls", "open_perms", "extended_socket_class", diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 7865926962ab..25c287324059 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -99,7 +99,7 @@ static void context_struct_compute_av(struct policydb *policydb, struct extended_perms *xperms); static int selinux_set_mapping(struct policydb *pol, - struct security_class_mapping *map, + const struct security_class_mapping *map, struct selinux_map *out_map) { u16 i, j; @@ -121,7 +121,7 @@ static int selinux_set_mapping(struct policydb *pol, /* Store the raw class and permission values */ j = 0; while (map[j].name) { - struct security_class_mapping *p_in = map + (j++); + const struct security_class_mapping *p_in = map + (j++); struct selinux_mapping *p_out = out_map->mapping + j; /* An empty class string skips ahead */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] selinux: declare data arrays const 2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche @ 2022-02-18 16:13 ` Paul Moore 2022-02-18 17:24 ` Nick Desaulniers 2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche 1 sibling, 1 reply; 24+ messages in thread From: Paul Moore @ 2022-02-18 16:13 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Jeremy Kerr, David S. Miller, Lakshmi Ramasubramanian, Yang Li, Austin Kim, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > The arrays for the policy capability names, the initial sid identifiers > and the class and permission names are not changed at runtime. Declare > them const to avoid accidental modification. > > The build time script genheaders needs to be exempted, since it converts > the entries to uppercase. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > scripts/selinux/genheaders/genheaders.c | 2 ++ > scripts/selinux/mdp/mdp.c | 4 ++-- > security/selinux/avc.c | 2 +- > security/selinux/include/avc_ss.h | 2 +- > security/selinux/include/classmap.h | 8 +++++++- > security/selinux/include/initial_sid_to_string.h | 9 ++++++++- > security/selinux/include/policycap.h | 2 +- > security/selinux/include/policycap_names.h | 2 +- > security/selinux/ss/services.c | 4 ++-- > 9 files changed, 25 insertions(+), 10 deletions(-) ... > diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c > index f355b3e0e968..5f7c0b7d9260 100644 > --- a/scripts/selinux/genheaders/genheaders.c > +++ b/scripts/selinux/genheaders/genheaders.c > @@ -15,6 +15,8 @@ struct security_class_mapping { > const char *perms[sizeof(unsigned) * 8 + 1]; > }; > > +/* Allow to convert entries in mappings to uppercase */ > +#define __SELINUX_GENHEADERS__ > #include "classmap.h" > #include "initial_sid_to_string.h" ... > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 35aac62a662e..07ade4af85ff 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -2,6 +2,12 @@ > #include <linux/capability.h> > #include <linux/socket.h> > > +#ifdef __SELINUX_GENHEADERS__ > +# define const_qual > +#else > +# define const_qual const > +#endif > + > #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ > "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" > > @@ -38,7 +44,7 @@ > * Note: The name for any socket class should be suffixed by "socket", > * and doesn't contain more than one substr of "socket". > */ > -struct security_class_mapping secclass_map[] = { > +const_qual struct security_class_mapping secclass_map[] = { > { "security", > { "compute_av", "compute_create", "compute_member", > "check_context", "load_policy", "compute_relabel", ... > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index 5d332aeb8b6c..915283cd89bd 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -1,5 +1,12 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > -static const char *initial_sid_to_string[] = > + > +#ifdef __SELINUX_GENHEADERS__ > +# define const_qual > +#else > +# define const_qual const > +#endif > + > +static const char *const_qual initial_sid_to_string[] = > { > NULL, > "kernel", Thanks for this Christian. I generally like when we can const'ify things like this, but I'm not excited about the const_qual hack on core SELinux kernel code to satisfy genheaders.c. I understand why it is needed, but I would rather clutter the genheaders.c code than the core SELinux kernel code. If we can't cast away the const'ification in genheaders.c could we simply allocate duplicate arrays in genheaders.c and store the transformed strings into the new arrays? -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] selinux: declare data arrays const 2022-02-18 16:13 ` Paul Moore @ 2022-02-18 17:24 ` Nick Desaulniers 2022-02-22 23:16 ` Paul Moore 0 siblings, 1 reply; 24+ messages in thread From: Nick Desaulniers @ 2022-02-18 17:24 UTC (permalink / raw) To: Paul Moore Cc: Christian Göttsche, selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Jeremy Kerr, David S. Miller, Lakshmi Ramasubramanian, Yang Li, Austin Kim, linux-kernel, llvm On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > > index 5d332aeb8b6c..915283cd89bd 100644 > > --- a/security/selinux/include/initial_sid_to_string.h > > +++ b/security/selinux/include/initial_sid_to_string.h > > @@ -1,5 +1,12 @@ > > /* SPDX-License-Identifier: GPL-2.0 */ > > -static const char *initial_sid_to_string[] = > > + > > +#ifdef __SELINUX_GENHEADERS__ > > +# define const_qual > > +#else > > +# define const_qual const > > +#endif > > + > > +static const char *const_qual initial_sid_to_string[] = > > { > > NULL, > > "kernel", > > Thanks for this Christian. I generally like when we can const'ify > things like this, but I'm not excited about the const_qual hack on > core SELinux kernel code to satisfy genheaders.c. I understand why it > is needed, but I would rather clutter the genheaders.c code than the > core SELinux kernel code. If we can't cast away the const'ification > in genheaders.c could we simply allocate duplicate arrays in > genheaders.c and store the transformed strings into the new arrays? Note: casting off const is UB. I've had to fix multiple bugs where clang will drop writes to variables declared const but had const'ness casted away. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] selinux: declare data arrays const 2022-02-18 17:24 ` Nick Desaulniers @ 2022-02-22 23:16 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-02-22 23:16 UTC (permalink / raw) To: Nick Desaulniers Cc: Christian Göttsche, selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Jeremy Kerr, David S. Miller, Lakshmi Ramasubramanian, Yang Li, Austin Kim, linux-kernel, llvm On Fri, Feb 18, 2022 at 12:24 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > > > index 5d332aeb8b6c..915283cd89bd 100644 > > > --- a/security/selinux/include/initial_sid_to_string.h > > > +++ b/security/selinux/include/initial_sid_to_string.h > > > @@ -1,5 +1,12 @@ > > > /* SPDX-License-Identifier: GPL-2.0 */ > > > -static const char *initial_sid_to_string[] = > > > + > > > +#ifdef __SELINUX_GENHEADERS__ > > > +# define const_qual > > > +#else > > > +# define const_qual const > > > +#endif > > > + > > > +static const char *const_qual initial_sid_to_string[] = > > > { > > > NULL, > > > "kernel", > > > > Thanks for this Christian. I generally like when we can const'ify > > things like this, but I'm not excited about the const_qual hack on > > core SELinux kernel code to satisfy genheaders.c. I understand why it > > is needed, but I would rather clutter the genheaders.c code than the > > core SELinux kernel code. If we can't cast away the const'ification > > in genheaders.c could we simply allocate duplicate arrays in > > genheaders.c and store the transformed strings into the new arrays? > > Note: casting off const is UB. I've had to fix multiple bugs where > clang will drop writes to variables declared const but had const'ness > casted away. Then let's just memcpy the array in genheaders.c. I'm okay with genheaders being a little ugly if it helps keep the core code cleaner. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 4/5] selinux: declare data arrays const 2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche 2022-02-18 16:13 ` Paul Moore @ 2022-03-08 16:55 ` Christian Göttsche 2022-04-04 20:03 ` Paul Moore 2022-05-02 14:43 ` [PATCH v3] " Christian Göttsche 1 sibling, 2 replies; 24+ messages in thread From: Christian Göttsche @ 2022-03-08 16:55 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Ondrej Mosnacek, David S. Miller, Jeremy Kerr, Richard Haines, Lakshmi Ramasubramanian, Austin Kim, Yang Li, linux-kernel The arrays for the policy capability names, the initial sid identifiers and the class and permission names are not changed at runtime. Declare them const to avoid accidental modification. Do not override the classmap and the initial sid list in the build time script genheaders, by using a static buffer in the conversion function stoupperx(). In cases we need to compare or print more than one identifier allocate a temporary copy. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- v2: Drop const exemption for genheaders script by rewriting stoupperx(). --- scripts/selinux/genheaders/genheaders.c | 76 ++++++++++--------- scripts/selinux/mdp/mdp.c | 4 +- security/selinux/avc.c | 2 +- security/selinux/include/avc_ss.h | 2 +- security/selinux/include/classmap.h | 2 +- .../selinux/include/initial_sid_to_string.h | 3 +- security/selinux/include/policycap.h | 2 +- security/selinux/include/policycap_names.h | 2 +- security/selinux/ss/services.c | 4 +- 9 files changed, 51 insertions(+), 46 deletions(-) diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index f355b3e0e968..a2caff3c997f 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -26,19 +26,23 @@ static void usage(void) exit(1); } -static char *stoupperx(const char *s) +static const char *stoupperx(const char *s) { - char *s2 = strdup(s); - char *p; + static char buffer[256]; + unsigned int i; + char *p = buffer; - if (!s2) { - fprintf(stderr, "%s: out of memory\n", progname); + for (i = 0; i < (sizeof(buffer) - 1) && *s; i++) + *p++ = toupper(*s++); + + if (*s) { + fprintf(stderr, "%s: buffer too small\n", progname); exit(3); } - for (p = s2; *p; p++) - *p = toupper(*p); - return s2; + *p = '\0'; + + return buffer; } int main(int argc, char *argv[]) @@ -59,35 +63,19 @@ int main(int argc, char *argv[]) exit(2); } - for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - map->name = stoupperx(map->name); - for (j = 0; map->perms[j]; j++) - map->perms[j] = stoupperx(map->perms[j]); - } - - isids_len = sizeof(initial_sid_to_string) / sizeof (char *); - for (i = 1; i < isids_len; i++) { - const char *s = initial_sid_to_string[i]; - - if (s) - initial_sid_to_string[i] = stoupperx(s); - } - fprintf(fout, "/* This file is automatically generated. Do not edit. */\n"); fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define _SELINUX_FLASK_H_\n\n"); - for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - fprintf(fout, "#define SECCLASS_%-39s %2d\n", map->name, i+1); - } + for (i = 0; secclass_map[i].name; i++) + fprintf(fout, "#define SECCLASS_%-39s %2d\n", stoupperx(secclass_map[i].name), i+1); fprintf(fout, "\n"); + isids_len = sizeof(initial_sid_to_string) / sizeof(char *); for (i = 1; i < isids_len; i++) { const char *s = initial_sid_to_string[i]; if (s) - fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i); + fprintf(fout, "#define SECINITSID_%-39s %2d\n", stoupperx(s), i); } fprintf(fout, "\n#define SECINITSID_NUM %d\n", i-1); fprintf(fout, "\nstatic inline bool security_is_socket_class(u16 kern_tclass)\n"); @@ -96,10 +84,18 @@ int main(int argc, char *argv[]) fprintf(fout, "\tswitch (kern_tclass) {\n"); for (i = 0; secclass_map[i].name; i++) { static char s[] = "SOCKET"; - struct security_class_mapping *map = &secclass_map[i]; - int len = strlen(map->name), l = sizeof(s) - 1; - if (len >= l && memcmp(map->name + len - l, s, l) == 0) - fprintf(fout, "\tcase SECCLASS_%s:\n", map->name); + int len, l; + char *name = strdup(stoupperx(secclass_map[i].name)); + + if (!name) { + fprintf(stderr, "%s: out of memory\n", progname); + exit(3); + } + len = strlen(name); + l = sizeof(s) - 1; + if (len >= l && memcmp(name + len - l, s, l) == 0) + fprintf(fout, "\tcase SECCLASS_%s:\n", name); + free(name); } fprintf(fout, "\t\tsock = true;\n"); fprintf(fout, "\t\tbreak;\n"); @@ -123,17 +119,25 @@ int main(int argc, char *argv[]) fprintf(fout, "#ifndef _SELINUX_AV_PERMISSIONS_H_\n#define _SELINUX_AV_PERMISSIONS_H_\n\n"); for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - int len = strlen(map->name); + const struct security_class_mapping *map = &secclass_map[i]; + int len; + char *name = strdup(stoupperx(map->name)); + + if (!name) { + fprintf(stderr, "%s: out of memory\n", progname); + exit(3); + } + len = strlen(name); for (j = 0; map->perms[j]; j++) { if (j >= 32) { fprintf(stderr, "Too many permissions to fit into an access vector at (%s, %s).\n", map->name, map->perms[j]); exit(5); } - fprintf(fout, "#define %s__%-*s 0x%08xU\n", map->name, - 39-len, map->perms[j], 1U<<j); + fprintf(fout, "#define %s__%-*s 0x%08xU\n", name, + 39-len, stoupperx(map->perms[j]), 1U<<j); } + free(name); } fprintf(fout, "\n#endif\n"); diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c index 105c1c31a316..1415604c3d24 100644 --- a/scripts/selinux/mdp/mdp.c +++ b/scripts/selinux/mdp/mdp.c @@ -82,7 +82,7 @@ int main(int argc, char *argv[]) /* print out the class permissions */ for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "class %s\n", map->name); fprintf(fout, "{\n"); for (j = 0; map->perms[j]; j++) @@ -103,7 +103,7 @@ int main(int argc, char *argv[]) #define SYSTEMLOW "s0" #define SYSTEMHIGH "s1:c0.c1" for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "mlsconstrain %s {\n", map->name); for (j = 0; map->perms[j]; j++) diff --git a/security/selinux/avc.c b/security/selinux/avc.c index abcd9740d10f..020985a53d8f 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -668,7 +668,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) struct common_audit_data *ad = a; struct selinux_audit_data *sad = ad->selinux_audit_data; u32 av = sad->audited; - const char **perms; + const char *const *perms; int i, perm; audit_log_format(ab, "avc: %s ", sad->denied ? "denied" : "granted"); diff --git a/security/selinux/include/avc_ss.h b/security/selinux/include/avc_ss.h index 88c384c5c09e..b38974e22d81 100644 --- a/security/selinux/include/avc_ss.h +++ b/security/selinux/include/avc_ss.h @@ -18,7 +18,7 @@ struct security_class_mapping { const char *perms[sizeof(u32) * 8 + 1]; }; -extern struct security_class_mapping secclass_map[]; +extern const struct security_class_mapping secclass_map[]; #endif /* _SELINUX_AVC_SS_H_ */ diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 35aac62a662e..ff757ae5f253 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -38,7 +38,7 @@ * Note: The name for any socket class should be suffixed by "socket", * and doesn't contain more than one substr of "socket". */ -struct security_class_mapping secclass_map[] = { +const struct security_class_mapping secclass_map[] = { { "security", { "compute_av", "compute_create", "compute_member", "check_context", "load_policy", "compute_relabel", diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 5d332aeb8b6c..05cc51085437 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ -static const char *initial_sid_to_string[] = + +static const char *const initial_sid_to_string[] = { NULL, "kernel", diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index 2680aa21205c..f35d3458e71d 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -16,6 +16,6 @@ enum { }; #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1) -extern const char *selinux_policycap_names[__POLICYDB_CAP_MAX]; +extern const char *const selinux_policycap_names[__POLICYDB_CAP_MAX]; #endif /* _SELINUX_POLICYCAP_H_ */ diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index 100da7d043db..2a87fc3702b8 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -5,7 +5,7 @@ #include "policycap.h" /* Policy capability names */ -const char *selinux_policycap_names[__POLICYDB_CAP_MAX] = { +const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = { "network_peer_controls", "open_perms", "extended_socket_class", diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 7865926962ab..25c287324059 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -99,7 +99,7 @@ static void context_struct_compute_av(struct policydb *policydb, struct extended_perms *xperms); static int selinux_set_mapping(struct policydb *pol, - struct security_class_mapping *map, + const struct security_class_mapping *map, struct selinux_map *out_map) { u16 i, j; @@ -121,7 +121,7 @@ static int selinux_set_mapping(struct policydb *pol, /* Store the raw class and permission values */ j = 0; while (map[j].name) { - struct security_class_mapping *p_in = map + (j++); + const struct security_class_mapping *p_in = map + (j++); struct selinux_mapping *p_out = out_map->mapping + j; /* An empty class string skips ahead */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/5] selinux: declare data arrays const 2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche @ 2022-04-04 20:03 ` Paul Moore 2022-05-02 14:43 ` [PATCH v3] " Christian Göttsche 1 sibling, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-04-04 20:03 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Ondrej Mosnacek, David S. Miller, Jeremy Kerr, Richard Haines, Lakshmi Ramasubramanian, Austin Kim, Yang Li, linux-kernel On Tue, Mar 8, 2022 at 11:55 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > The arrays for the policy capability names, the initial sid identifiers > and the class and permission names are not changed at runtime. Declare > them const to avoid accidental modification. > > Do not override the classmap and the initial sid list in the build time > script genheaders, by using a static buffer in the conversion function > stoupperx(). In cases we need to compare or print more than one > identifier allocate a temporary copy. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > v2: > Drop const exemption for genheaders script by rewriting stoupperx(). > --- > scripts/selinux/genheaders/genheaders.c | 76 ++++++++++--------- > scripts/selinux/mdp/mdp.c | 4 +- > security/selinux/avc.c | 2 +- > security/selinux/include/avc_ss.h | 2 +- > security/selinux/include/classmap.h | 2 +- > .../selinux/include/initial_sid_to_string.h | 3 +- > security/selinux/include/policycap.h | 2 +- > security/selinux/include/policycap_names.h | 2 +- > security/selinux/ss/services.c | 4 +- > 9 files changed, 51 insertions(+), 46 deletions(-) > > diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c > index f355b3e0e968..a2caff3c997f 100644 > --- a/scripts/selinux/genheaders/genheaders.c > +++ b/scripts/selinux/genheaders/genheaders.c > @@ -26,19 +26,23 @@ static void usage(void) > exit(1); > } > > -static char *stoupperx(const char *s) > +static const char *stoupperx(const char *s) > { > - char *s2 = strdup(s); > - char *p; > + static char buffer[256]; > + unsigned int i; > + char *p = buffer; > > - if (!s2) { > - fprintf(stderr, "%s: out of memory\n", progname); > + for (i = 0; i < (sizeof(buffer) - 1) && *s; i++) > + *p++ = toupper(*s++); > + > + if (*s) { > + fprintf(stderr, "%s: buffer too small\n", progname); > exit(3); > } > > - for (p = s2; *p; p++) > - *p = toupper(*p); > - return s2; > + *p = '\0'; > + > + return buffer; > } Hmmm. I recognize this is just build time code so it's not as critical, but I still don't like the idea of passing back a static buffer to the caller; it just seems like we are asking for future trouble. I'm also curious as to why you made this choice in this revision when the existing code should have worked (passed a const, returned a non-const). I'm sure I'm missing something obvious, but can you help me understand why this is necessary? -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] selinux: declare data arrays const 2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche 2022-04-04 20:03 ` Paul Moore @ 2022-05-02 14:43 ` Christian Göttsche 2022-05-03 19:59 ` Paul Moore 1 sibling, 1 reply; 24+ messages in thread From: Christian Göttsche @ 2022-05-02 14:43 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Ondrej Mosnacek, David S. Miller, Jeremy Kerr, Richard Haines, Xiu Jianfeng, Nick Desaulniers, Jiapeng Chong, Michal Orzel, Yang Li, Austin Kim, linux-kernel The arrays for the policy capability names, the initial sid identifiers and the class and permission names are not changed at runtime. Declare them const to avoid accidental modification. Do not override the classmap and the initial sid list in the build time script genheaders. Check flose(3) is successful in genheaders.c, otherwise the written data might be corrupted or incomplete. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- v2: Drop const exemption for genheaders script by rewriting stoupperx(). v3: - Declare some additional data array const - Do not use static buffer in genheaders.c::stoupperx() - Check fclose(3) in genheaders.c --- scripts/selinux/genheaders/genheaders.c | 75 +++++++++++-------- scripts/selinux/mdp/mdp.c | 4 +- security/selinux/avc.c | 2 +- security/selinux/include/avc_ss.h | 2 +- security/selinux/include/classmap.h | 2 +- .../selinux/include/initial_sid_to_string.h | 4 +- security/selinux/include/policycap.h | 2 +- security/selinux/include/policycap_names.h | 2 +- security/selinux/ss/avtab.c | 2 +- security/selinux/ss/policydb.c | 36 ++++----- security/selinux/ss/services.c | 4 +- 11 files changed, 72 insertions(+), 63 deletions(-) diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index f355b3e0e968..15520806889e 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -59,35 +59,27 @@ int main(int argc, char *argv[]) exit(2); } - for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - map->name = stoupperx(map->name); - for (j = 0; map->perms[j]; j++) - map->perms[j] = stoupperx(map->perms[j]); - } - - isids_len = sizeof(initial_sid_to_string) / sizeof (char *); - for (i = 1; i < isids_len; i++) { - const char *s = initial_sid_to_string[i]; - - if (s) - initial_sid_to_string[i] = stoupperx(s); - } - fprintf(fout, "/* This file is automatically generated. Do not edit. */\n"); fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define _SELINUX_FLASK_H_\n\n"); for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - fprintf(fout, "#define SECCLASS_%-39s %2d\n", map->name, i+1); + char *name = stoupperx(secclass_map[i].name); + + fprintf(fout, "#define SECCLASS_%-39s %2d\n", name, i+1); + free(name); } fprintf(fout, "\n"); + isids_len = sizeof(initial_sid_to_string) / sizeof(char *); for (i = 1; i < isids_len; i++) { const char *s = initial_sid_to_string[i]; - if (s) - fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i); + if (s) { + char *sidname = stoupperx(s); + + fprintf(fout, "#define SECINITSID_%-39s %2d\n", sidname, i); + free(sidname); + } } fprintf(fout, "\n#define SECINITSID_NUM %d\n", i-1); fprintf(fout, "\nstatic inline bool security_is_socket_class(u16 kern_tclass)\n"); @@ -96,10 +88,14 @@ int main(int argc, char *argv[]) fprintf(fout, "\tswitch (kern_tclass) {\n"); for (i = 0; secclass_map[i].name; i++) { static char s[] = "SOCKET"; - struct security_class_mapping *map = &secclass_map[i]; - int len = strlen(map->name), l = sizeof(s) - 1; - if (len >= l && memcmp(map->name + len - l, s, l) == 0) - fprintf(fout, "\tcase SECCLASS_%s:\n", map->name); + int len, l; + char *name = stoupperx(secclass_map[i].name); + + len = strlen(name); + l = sizeof(s) - 1; + if (len >= l && memcmp(name + len - l, s, l) == 0) + fprintf(fout, "\tcase SECCLASS_%s:\n", name); + free(name); } fprintf(fout, "\t\tsock = true;\n"); fprintf(fout, "\t\tbreak;\n"); @@ -110,33 +106,52 @@ int main(int argc, char *argv[]) fprintf(fout, "}\n"); fprintf(fout, "\n#endif\n"); - fclose(fout); + + if (fclose(fout) != 0) { + fprintf(stderr, "Could not successfully close %s: %s\n", + argv[1], strerror(errno)); + exit(4); + } fout = fopen(argv[2], "w"); if (!fout) { fprintf(stderr, "Could not open %s for writing: %s\n", argv[2], strerror(errno)); - exit(4); + exit(5); } fprintf(fout, "/* This file is automatically generated. Do not edit. */\n"); fprintf(fout, "#ifndef _SELINUX_AV_PERMISSIONS_H_\n#define _SELINUX_AV_PERMISSIONS_H_\n\n"); for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; - int len = strlen(map->name); + const struct security_class_mapping *map = &secclass_map[i]; + int len; + char *name = stoupperx(map->name); + + len = strlen(name); for (j = 0; map->perms[j]; j++) { + char *permname; + if (j >= 32) { fprintf(stderr, "Too many permissions to fit into an access vector at (%s, %s).\n", map->name, map->perms[j]); exit(5); } - fprintf(fout, "#define %s__%-*s 0x%08xU\n", map->name, - 39-len, map->perms[j], 1U<<j); + permname = stoupperx(map->perms[j]); + fprintf(fout, "#define %s__%-*s 0x%08xU\n", name, + 39-len, permname, 1U<<j); + free(permname); } + free(name); } fprintf(fout, "\n#endif\n"); - fclose(fout); + + if (fclose(fout) != 0) { + fprintf(stderr, "Could not successfully close %s: %s\n", + argv[2], strerror(errno)); + exit(6); + } + exit(0); } diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c index 105c1c31a316..1415604c3d24 100644 --- a/scripts/selinux/mdp/mdp.c +++ b/scripts/selinux/mdp/mdp.c @@ -82,7 +82,7 @@ int main(int argc, char *argv[]) /* print out the class permissions */ for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "class %s\n", map->name); fprintf(fout, "{\n"); for (j = 0; map->perms[j]; j++) @@ -103,7 +103,7 @@ int main(int argc, char *argv[]) #define SYSTEMLOW "s0" #define SYSTEMHIGH "s1:c0.c1" for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "mlsconstrain %s {\n", map->name); for (j = 0; map->perms[j]; j++) diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 874c1c6fe10b..9a43af0ebd7d 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -668,7 +668,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) struct common_audit_data *ad = a; struct selinux_audit_data *sad = ad->selinux_audit_data; u32 av = sad->audited; - const char **perms; + const char *const *perms; int i, perm; audit_log_format(ab, "avc: %s ", sad->denied ? "denied" : "granted"); diff --git a/security/selinux/include/avc_ss.h b/security/selinux/include/avc_ss.h index 88c384c5c09e..b38974e22d81 100644 --- a/security/selinux/include/avc_ss.h +++ b/security/selinux/include/avc_ss.h @@ -18,7 +18,7 @@ struct security_class_mapping { const char *perms[sizeof(u32) * 8 + 1]; }; -extern struct security_class_mapping secclass_map[]; +extern const struct security_class_mapping secclass_map[]; #endif /* _SELINUX_AVC_SS_H_ */ diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 35aac62a662e..ff757ae5f253 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -38,7 +38,7 @@ * Note: The name for any socket class should be suffixed by "socket", * and doesn't contain more than one substr of "socket". */ -struct security_class_mapping secclass_map[] = { +const struct security_class_mapping secclass_map[] = { { "security", { "compute_av", "compute_create", "compute_member", "check_context", "load_policy", "compute_relabel", diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 5d332aeb8b6c..7942bd5db78c 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ -static const char *initial_sid_to_string[] = -{ + +static const char *const initial_sid_to_string[] = { NULL, "kernel", "security", diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index 2680aa21205c..f35d3458e71d 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -16,6 +16,6 @@ enum { }; #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1) -extern const char *selinux_policycap_names[__POLICYDB_CAP_MAX]; +extern const char *const selinux_policycap_names[__POLICYDB_CAP_MAX]; #endif /* _SELINUX_POLICYCAP_H_ */ diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index 100da7d043db..2a87fc3702b8 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -5,7 +5,7 @@ #include "policycap.h" /* Policy capability names */ -const char *selinux_policycap_names[__POLICYDB_CAP_MAX] = { +const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = { "network_peer_controls", "open_perms", "extended_socket_class", diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index cfdae20792e1..88eef0128773 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -385,7 +385,7 @@ void avtab_hash_eval(struct avtab *h, char *tag) chain2_len_sum); } -static uint16_t spec_order[] = { +static const uint16_t spec_order[] = { AVTAB_ALLOWED, AVTAB_AUDITDENY, AVTAB_AUDITALLOW, diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index d036e1238e77..db795ca433a7 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -61,7 +61,7 @@ struct policydb_compat_info { }; /* These need to be updated if SYM_NUM or OCON_NUM changes */ -static struct policydb_compat_info policydb_compat[] = { +static const struct policydb_compat_info policydb_compat[] = { { .version = POLICYDB_VERSION_BASE, .sym_num = SYM_NUM - 3, @@ -159,18 +159,16 @@ static struct policydb_compat_info policydb_compat[] = { }, }; -static struct policydb_compat_info *policydb_lookup_compat(int version) +static const struct policydb_compat_info *policydb_lookup_compat(int version) { int i; - struct policydb_compat_info *info = NULL; for (i = 0; i < ARRAY_SIZE(policydb_compat); i++) { - if (policydb_compat[i].version == version) { - info = &policydb_compat[i]; - break; - } + if (policydb_compat[i].version == version) + return &policydb_compat[i]; } - return info; + + return NULL; } /* @@ -314,8 +312,7 @@ static int cat_destroy(void *key, void *datum, void *p) return 0; } -static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = -{ +static int (*const destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = { common_destroy, cls_destroy, role_destroy, @@ -670,8 +667,7 @@ static int cat_index(void *key, void *datum, void *datap) return 0; } -static int (*index_f[SYM_NUM]) (void *key, void *datum, void *datap) = -{ +static int (*const index_f[SYM_NUM]) (void *key, void *datum, void *datap) = { common_index, class_index, role_index, @@ -1639,8 +1635,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp) return rc; } -static int (*read_f[SYM_NUM]) (struct policydb *p, struct symtab *s, void *fp) = -{ +static int (*const read_f[SYM_NUM]) (struct policydb *p, struct symtab *s, void *fp) = { common_read, class_read, role_read, @@ -2211,7 +2206,7 @@ static int genfs_read(struct policydb *p, void *fp) return rc; } -static int ocontext_read(struct policydb *p, struct policydb_compat_info *info, +static int ocontext_read(struct policydb *p, const struct policydb_compat_info *info, void *fp) { int i, j, rc; @@ -2407,7 +2402,7 @@ int policydb_read(struct policydb *p, void *fp) u32 len, nprim, nel, perm; char *policydb_str; - struct policydb_compat_info *info; + const struct policydb_compat_info *info; policydb_init(p); @@ -3241,9 +3236,8 @@ static int user_write(void *vkey, void *datum, void *ptr) return 0; } -static int (*write_f[SYM_NUM]) (void *key, void *datum, - void *datap) = -{ +static int (*const write_f[SYM_NUM]) (void *key, void *datum, + void *datap) = { common_write, class_write, role_write, @@ -3254,7 +3248,7 @@ static int (*write_f[SYM_NUM]) (void *key, void *datum, cat_write, }; -static int ocontext_write(struct policydb *p, struct policydb_compat_info *info, +static int ocontext_write(struct policydb *p, const struct policydb_compat_info *info, void *fp) { unsigned int i, j, rc; @@ -3611,7 +3605,7 @@ int policydb_write(struct policydb *p, void *fp) __le32 buf[4]; u32 config; size_t len; - struct policydb_compat_info *info; + const struct policydb_compat_info *info; /* * refuse to write policy older than compressed avtab diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 802a80648c6c..d59230258f6f 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -99,7 +99,7 @@ static void context_struct_compute_av(struct policydb *policydb, struct extended_perms *xperms); static int selinux_set_mapping(struct policydb *pol, - struct security_class_mapping *map, + const struct security_class_mapping *map, struct selinux_map *out_map) { u16 i, j; @@ -121,7 +121,7 @@ static int selinux_set_mapping(struct policydb *pol, /* Store the raw class and permission values */ j = 0; while (map[j].name) { - struct security_class_mapping *p_in = map + (j++); + const struct security_class_mapping *p_in = map + (j++); struct selinux_mapping *p_out = out_map->mapping + j; /* An empty class string skips ahead */ -- 2.36.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3] selinux: declare data arrays const 2022-05-02 14:43 ` [PATCH v3] " Christian Göttsche @ 2022-05-03 19:59 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-05-03 19:59 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Ondrej Mosnacek, David S. Miller, Jeremy Kerr, Richard Haines, Xiu Jianfeng, Nick Desaulniers, Jiapeng Chong, Michal Orzel, Yang Li, Austin Kim, linux-kernel On Mon, May 2, 2022 at 10:43 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > The arrays for the policy capability names, the initial sid identifiers > and the class and permission names are not changed at runtime. Declare > them const to avoid accidental modification. > > Do not override the classmap and the initial sid list in the build time > script genheaders. > > Check flose(3) is successful in genheaders.c, otherwise the written data > might be corrupted or incomplete. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > v2: > Drop const exemption for genheaders script by rewriting stoupperx(). > v3: > - Declare some additional data array const > - Do not use static buffer in genheaders.c::stoupperx() > - Check fclose(3) in genheaders.c > --- > scripts/selinux/genheaders/genheaders.c | 75 +++++++++++-------- > scripts/selinux/mdp/mdp.c | 4 +- > security/selinux/avc.c | 2 +- > security/selinux/include/avc_ss.h | 2 +- > security/selinux/include/classmap.h | 2 +- > .../selinux/include/initial_sid_to_string.h | 4 +- > security/selinux/include/policycap.h | 2 +- > security/selinux/include/policycap_names.h | 2 +- > security/selinux/ss/avtab.c | 2 +- > security/selinux/ss/policydb.c | 36 ++++----- > security/selinux/ss/services.c | 4 +- > 11 files changed, 72 insertions(+), 63 deletions(-) Thanks this revision is much better, merged into selinux/next. I did have to apply parts of this patch manually, so if you notice anything wrong with the commit please let me know. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/5] selinux: drop unnecessary NULL check 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche 2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche 2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche @ 2022-02-17 14:21 ` Christian Göttsche 2022-02-18 16:22 ` Paul Moore ` (2 more replies) 2022-02-17 14:21 ` [PATCH 1/5] selinux: drop return statement at end of void functions Christian Göttsche 2022-02-18 15:47 ` [PATCH 2/5] selinux: use correct type for context length Paul Moore 4 siblings, 3 replies; 24+ messages in thread From: Christian Göttsche @ 2022-02-17 14:21 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") introduced a NULL check on the context after a successful call to security_sid_to_context(). This is on the one hand redundant after checking for success and on the other hand insufficient on an actual NULL pointer, since the context is passed to seq_escape() leading to a call of strlen() on it. Reported by Clang analyzer: In file included from security/selinux/hooks.c:28: In file included from ./include/linux/tracehook.h:50: In file included from ./include/linux/memcontrol.h:13: In file included from ./include/linux/cgroup.h:18: ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] seq_escape_mem(m, src, strlen(src), flags, esc); ^~~~~~~~~~~ Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/hooks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1e69f88eb326..ac802b99d36c 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) rc = security_sid_to_context(&selinux_state, sid, &context, &len); if (!rc) { - bool has_comma = context && strchr(context, ','); + bool has_comma = strchr(context, ','); seq_putc(m, '='); if (has_comma) -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche @ 2022-02-18 16:22 ` Paul Moore 2022-02-18 17:31 ` Nick Desaulniers 2022-06-07 21:22 ` Paul Moore 2 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-02-18 16:22 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > introduced a NULL check on the context after a successful call to > security_sid_to_context(). This is on the one hand redundant after > checking for success and on the other hand insufficient on an actual > NULL pointer, since the context is passed to seq_escape() leading to a > call of strlen() on it. > > Reported by Clang analyzer: > > In file included from security/selinux/hooks.c:28: > In file included from ./include/linux/tracehook.h:50: > In file included from ./include/linux/memcontrol.h:13: > In file included from ./include/linux/cgroup.h:18: > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > seq_escape_mem(m, src, strlen(src), flags, esc); > ^~~~~~~~~~~ Interesting. If I'm understanding this correctly, Clang is reporting on a potential NULL pointer simply because we are checking for a NULL pointer a few lines earlier, even though @context should not be NULL if (rc != 0)? > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 1e69f88eb326..ac802b99d36c 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) > rc = security_sid_to_context(&selinux_state, sid, > &context, &len); > if (!rc) { > - bool has_comma = context && strchr(context, ','); > + bool has_comma = strchr(context, ','); > > seq_putc(m, '='); > if (has_comma) > -- > 2.35.1 -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche 2022-02-18 16:22 ` Paul Moore @ 2022-02-18 17:31 ` Nick Desaulniers 2022-03-08 16:09 ` Christian Göttsche 2022-06-07 21:22 ` Paul Moore 2 siblings, 1 reply; 24+ messages in thread From: Nick Desaulniers @ 2022-02-18 17:31 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > introduced a NULL check on the context after a successful call to > security_sid_to_context(). This is on the one hand redundant after > checking for success and on the other hand insufficient on an actual > NULL pointer, since the context is passed to seq_escape() leading to a > call of strlen() on it. > > Reported by Clang analyzer: > > In file included from security/selinux/hooks.c:28: > In file included from ./include/linux/tracehook.h:50: > In file included from ./include/linux/memcontrol.h:13: > In file included from ./include/linux/cgroup.h:18: > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > seq_escape_mem(m, src, strlen(src), flags, esc); > ^~~~~~~~~~~ I'm guessing there was more to this trace for this instance of this warning? > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 1e69f88eb326..ac802b99d36c 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) > rc = security_sid_to_context(&selinux_state, sid, > &context, &len); > if (!rc) { ^ perhaps changing this condition to: if (!rc && context) { It might be nice to retain the null ptr check should the semantics of security_sid_to_context ever change. > - bool has_comma = context && strchr(context, ','); > + bool has_comma = strchr(context, ','); > > seq_putc(m, '='); > if (has_comma) > -- > 2.35.1 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-02-18 17:31 ` Nick Desaulniers @ 2022-03-08 16:09 ` Christian Göttsche 2022-05-02 13:43 ` Christian Göttsche 0 siblings, 1 reply; 24+ messages in thread From: Christian Göttsche @ 2022-03-08 16:09 UTC (permalink / raw) To: Nick Desaulniers Cc: SElinux list, Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, Linux kernel mailing list, llvm On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > > introduced a NULL check on the context after a successful call to > > security_sid_to_context(). This is on the one hand redundant after > > checking for success and on the other hand insufficient on an actual > > NULL pointer, since the context is passed to seq_escape() leading to a > > call of strlen() on it. > > > > Reported by Clang analyzer: > > > > In file included from security/selinux/hooks.c:28: > > In file included from ./include/linux/tracehook.h:50: > > In file included from ./include/linux/memcontrol.h:13: > > In file included from ./include/linux/cgroup.h:18: > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > > seq_escape_mem(m, src, strlen(src), flags, esc); > > ^~~~~~~~~~~ > > I'm guessing there was more to this trace for this instance of this warning? Yes, complete output appended at the end. > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/hooks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 1e69f88eb326..ac802b99d36c 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) > > rc = security_sid_to_context(&selinux_state, sid, > > &context, &len); > > if (!rc) { > > ^ perhaps changing this condition to: > > if (!rc && context) { > > It might be nice to retain the null ptr check should the semantics of > security_sid_to_context ever change. If I read the implementation of security_sid_to_context() and its callees correctly it should never return 0 (success) and not have populated its 3 argument, unless the passed pointer was zero, which by passing the address of a stack variable - &context - is not the case). > > > - bool has_comma = context && strchr(context, ','); > > + bool has_comma = strchr(context, ','); > > > > seq_putc(m, '='); > > if (has_comma) > > -- > > 2.35.1 > > > > > -- > Thanks, > ~Nick Desaulniers clang-tidy report: ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [clang-analyzer-unix.cstring.NullArg] seq_escape_mem(m, src, strlen(src), flags, esc); ^ ./security/selinux/hooks.c:1041:6: note: Assuming the condition is false if (!(sbsec->flags & SE_SBINITIALIZED)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./security/selinux/hooks.c:1041:2: note: Taking false branch if (!(sbsec->flags & SE_SBINITIALIZED)) ^ ./security/selinux/hooks.c:1044:6: note: Assuming the condition is false if (!selinux_initialized(&selinux_state)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./security/selinux/hooks.c:1044:2: note: Taking false branch if (!selinux_initialized(&selinux_state)) ^ ./security/selinux/hooks.c:1047:6: note: Assuming the condition is true if (sbsec->flags & FSCONTEXT_MNT) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./security/selinux/hooks.c:1047:2: note: Taking true branch if (sbsec->flags & FSCONTEXT_MNT) { ^ ./security/selinux/hooks.c:1050:8: note: Calling 'show_sid' rc = show_sid(m, sbsec->sid); ^~~~~~~~~~~~~~~~~~~~~~~ ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context' rc = security_sid_to_context(&selinux_state, sid, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0 if (!rc) { ^~~ ./security/selinux/hooks.c:1022:2: note: Taking true branch if (!rc) { ^ ./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null bool has_comma = context && strchr(context, ','); ^~~~~~~ ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false bool has_comma = context && strchr(context, ','); ^ ./security/selinux/hooks.c:1026:7: note: 'has_comma' is false if (has_comma) ^~~~~~~~~ ./security/selinux/hooks.c:1026:3: note: Taking false branch if (has_comma) ^ ./security/selinux/hooks.c:1028:17: note: Passing null pointer value via 2nd parameter 's' seq_escape(m, context, "\"\n\\"); ^~~~~~~ ./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape' seq_escape(m, context, "\"\n\\"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ././include/linux/seq_file.h:152:20: note: Passing null pointer value via 2nd parameter 'src' seq_escape_str(m, s, ESCAPE_OCTAL, esc); ^ ././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str' seq_escape_str(m, s, ESCAPE_OCTAL, esc); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st argument to string length function seq_escape_mem(m, src, strlen(src), flags, esc); ^ ~~~ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-03-08 16:09 ` Christian Göttsche @ 2022-05-02 13:43 ` Christian Göttsche 2022-05-04 11:15 ` Ondrej Mosnacek 0 siblings, 1 reply; 24+ messages in thread From: Christian Göttsche @ 2022-05-02 13:43 UTC (permalink / raw) To: Nick Desaulniers Cc: SElinux list, Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, Linux kernel mailing list, llvm On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <cgzones@googlemail.com> wrote: > > On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > > > introduced a NULL check on the context after a successful call to > > > security_sid_to_context(). This is on the one hand redundant after > > > checking for success and on the other hand insufficient on an actual > > > NULL pointer, since the context is passed to seq_escape() leading to a > > > call of strlen() on it. > > > > > > Reported by Clang analyzer: > > > > > > In file included from security/selinux/hooks.c:28: > > > In file included from ./include/linux/tracehook.h:50: > > > In file included from ./include/linux/memcontrol.h:13: > > > In file included from ./include/linux/cgroup.h:18: > > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > > > seq_escape_mem(m, src, strlen(src), flags, esc); > > > ^~~~~~~~~~~ > > > > I'm guessing there was more to this trace for this instance of this warning? > > Yes, complete output appended at the end. > > > > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > --- > > > security/selinux/hooks.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 1e69f88eb326..ac802b99d36c 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) > > > rc = security_sid_to_context(&selinux_state, sid, > > > &context, &len); > > > if (!rc) { > > > > ^ perhaps changing this condition to: > > > > if (!rc && context) { > > > > It might be nice to retain the null ptr check should the semantics of > > security_sid_to_context ever change. > > If I read the implementation of security_sid_to_context() and its callees > correctly it should never return 0 (success) and not have populated its 3 > argument, unless the passed pointer was zero, which by passing the address > of a stack variable - &context - is not the case). > Kindly ping; is my analysis correct that after rc = security_sid_to_context(&selinux_state, sid, &context, &len); there is no possibility that rc is 0 AND context is NULL? > > > > > - bool has_comma = context && strchr(context, ','); > > > + bool has_comma = strchr(context, ','); > > > > > > seq_putc(m, '='); > > > if (has_comma) > > > -- > > > 2.35.1 > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers > > > clang-tidy report: > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st > argument to string length function > [clang-analyzer-unix.cstring.NullArg] > seq_escape_mem(m, src, strlen(src), flags, esc); > ^ > ./security/selinux/hooks.c:1041:6: note: Assuming the condition is false > if (!(sbsec->flags & SE_SBINITIALIZED)) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./security/selinux/hooks.c:1041:2: note: Taking false branch > if (!(sbsec->flags & SE_SBINITIALIZED)) > ^ > ./security/selinux/hooks.c:1044:6: note: Assuming the condition is false > if (!selinux_initialized(&selinux_state)) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./security/selinux/hooks.c:1044:2: note: Taking false branch > if (!selinux_initialized(&selinux_state)) > ^ > ./security/selinux/hooks.c:1047:6: note: Assuming the condition is true > if (sbsec->flags & FSCONTEXT_MNT) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./security/selinux/hooks.c:1047:2: note: Taking true branch > if (sbsec->flags & FSCONTEXT_MNT) { > ^ > ./security/selinux/hooks.c:1050:8: note: Calling 'show_sid' > rc = show_sid(m, sbsec->sid); > ^~~~~~~~~~~~~~~~~~~~~~~ > ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context' > rc = security_sid_to_context(&selinux_state, sid, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0 > if (!rc) { > ^~~ > ./security/selinux/hooks.c:1022:2: note: Taking true branch > if (!rc) { > ^ > ./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null > bool has_comma = context && strchr(context, ','); > ^~~~~~~ > ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false > bool has_comma = context && strchr(context, ','); > ^ > ./security/selinux/hooks.c:1026:7: note: 'has_comma' is false > if (has_comma) > ^~~~~~~~~ > ./security/selinux/hooks.c:1026:3: note: Taking false branch > if (has_comma) > ^ > ./security/selinux/hooks.c:1028:17: note: Passing null pointer value > via 2nd parameter 's' > seq_escape(m, context, "\"\n\\"); > ^~~~~~~ > ./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape' > seq_escape(m, context, "\"\n\\"); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ././include/linux/seq_file.h:152:20: note: Passing null pointer value > via 2nd parameter 'src' > seq_escape_str(m, s, ESCAPE_OCTAL, esc); > ^ > ././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str' > seq_escape_str(m, s, ESCAPE_OCTAL, esc); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st > argument to string length function > seq_escape_mem(m, src, strlen(src), flags, esc); > ^ ~~~ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-05-02 13:43 ` Christian Göttsche @ 2022-05-04 11:15 ` Ondrej Mosnacek 0 siblings, 0 replies; 24+ messages in thread From: Ondrej Mosnacek @ 2022-05-04 11:15 UTC (permalink / raw) To: Christian Göttsche Cc: Nick Desaulniers, SElinux list, Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, Linux kernel mailing list, llvm On Mon, May 2, 2022 at 3:43 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <cgzones@googlemail.com> wrote: > > > > On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche > > > <cgzones@googlemail.com> wrote: > > > > > > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > > > > introduced a NULL check on the context after a successful call to > > > > security_sid_to_context(). This is on the one hand redundant after > > > > checking for success and on the other hand insufficient on an actual > > > > NULL pointer, since the context is passed to seq_escape() leading to a > > > > call of strlen() on it. > > > > > > > > Reported by Clang analyzer: > > > > > > > > In file included from security/selinux/hooks.c:28: > > > > In file included from ./include/linux/tracehook.h:50: > > > > In file included from ./include/linux/memcontrol.h:13: > > > > In file included from ./include/linux/cgroup.h:18: > > > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > > > > seq_escape_mem(m, src, strlen(src), flags, esc); > > > > ^~~~~~~~~~~ > > > > > > I'm guessing there was more to this trace for this instance of this warning? > > > > Yes, complete output appended at the end. > > > > > > > > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > --- > > > > security/selinux/hooks.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 1e69f88eb326..ac802b99d36c 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid) > > > > rc = security_sid_to_context(&selinux_state, sid, > > > > &context, &len); > > > > if (!rc) { > > > > > > ^ perhaps changing this condition to: > > > > > > if (!rc && context) { > > > > > > It might be nice to retain the null ptr check should the semantics of > > > security_sid_to_context ever change. > > > > If I read the implementation of security_sid_to_context() and its callees > > correctly it should never return 0 (success) and not have populated its 3 > > argument, unless the passed pointer was zero, which by passing the address > > of a stack variable - &context - is not the case). > > > > Kindly ping; > is my analysis correct that after > > rc = security_sid_to_context(&selinux_state, sid, > &context, &len); > > there is no possibility that rc is 0 AND context is NULL? Yes, I think this patch is good. rc == 0 means success, which in this case means that a valid context string has been returned. Thus, there is no point in checking for NULL, other than being super-defensive about future changes to security_sid_to_context() messing something up (if we did this everywhere, then there would be NULL checks all over the place...). > > > > > > > > - bool has_comma = context && strchr(context, ','); > > > > + bool has_comma = strchr(context, ','); > > > > > > > > seq_putc(m, '='); > > > > if (has_comma) > > > > -- > > > > 2.35.1 > > > > > > > > > > > > > -- > > > Thanks, > > > ~Nick Desaulniers > > > > > > clang-tidy report: > > > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st > > argument to string length function > > [clang-analyzer-unix.cstring.NullArg] > > seq_escape_mem(m, src, strlen(src), flags, esc); > > ^ > > ./security/selinux/hooks.c:1041:6: note: Assuming the condition is false > > if (!(sbsec->flags & SE_SBINITIALIZED)) > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ./security/selinux/hooks.c:1041:2: note: Taking false branch > > if (!(sbsec->flags & SE_SBINITIALIZED)) > > ^ > > ./security/selinux/hooks.c:1044:6: note: Assuming the condition is false > > if (!selinux_initialized(&selinux_state)) > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ./security/selinux/hooks.c:1044:2: note: Taking false branch > > if (!selinux_initialized(&selinux_state)) > > ^ > > ./security/selinux/hooks.c:1047:6: note: Assuming the condition is true > > if (sbsec->flags & FSCONTEXT_MNT) { > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ./security/selinux/hooks.c:1047:2: note: Taking true branch > > if (sbsec->flags & FSCONTEXT_MNT) { > > ^ > > ./security/selinux/hooks.c:1050:8: note: Calling 'show_sid' > > rc = show_sid(m, sbsec->sid); > > ^~~~~~~~~~~~~~~~~~~~~~~ > > ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context' > > rc = security_sid_to_context(&selinux_state, sid, > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0 > > if (!rc) { > > ^~~ > > ./security/selinux/hooks.c:1022:2: note: Taking true branch > > if (!rc) { > > ^ > > ./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null > > bool has_comma = context && strchr(context, ','); > > ^~~~~~~ > > ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false > > bool has_comma = context && strchr(context, ','); > > ^ > > ./security/selinux/hooks.c:1026:7: note: 'has_comma' is false > > if (has_comma) > > ^~~~~~~~~ > > ./security/selinux/hooks.c:1026:3: note: Taking false branch > > if (has_comma) > > ^ > > ./security/selinux/hooks.c:1028:17: note: Passing null pointer value > > via 2nd parameter 's' > > seq_escape(m, context, "\"\n\\"); > > ^~~~~~~ > > ./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape' > > seq_escape(m, context, "\"\n\\"); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ././include/linux/seq_file.h:152:20: note: Passing null pointer value > > via 2nd parameter 'src' > > seq_escape_str(m, s, ESCAPE_OCTAL, esc); > > ^ > > ././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str' > > seq_escape_str(m, s, ESCAPE_OCTAL, esc); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st > > argument to string length function > > seq_escape_mem(m, src, strlen(src), flags, esc); > > ^ ~~~ > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche 2022-02-18 16:22 ` Paul Moore 2022-02-18 17:31 ` Nick Desaulniers @ 2022-06-07 21:22 ` Paul Moore 2022-06-07 21:26 ` Nick Desaulniers 2 siblings, 1 reply; 24+ messages in thread From: Paul Moore @ 2022-06-07 21:22 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > introduced a NULL check on the context after a successful call to > security_sid_to_context(). This is on the one hand redundant after > checking for success and on the other hand insufficient on an actual > NULL pointer, since the context is passed to seq_escape() leading to a > call of strlen() on it. > > Reported by Clang analyzer: > > In file included from security/selinux/hooks.c:28: > In file included from ./include/linux/tracehook.h:50: > In file included from ./include/linux/memcontrol.h:13: > In file included from ./include/linux/cgroup.h:18: > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > seq_escape_mem(m, src, strlen(src), flags, esc); > ^~~~~~~~~~~ > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I was waiting for Nick to reply, but he never did, and this looks good to me so I just merged it into selinux/next. Thanks for your patience Christian. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-06-07 21:22 ` Paul Moore @ 2022-06-07 21:26 ` Nick Desaulniers 2022-06-07 21:35 ` Paul Moore 0 siblings, 1 reply; 24+ messages in thread From: Nick Desaulniers @ 2022-06-07 21:26 UTC (permalink / raw) To: Paul Moore Cc: Christian Göttsche, selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > > introduced a NULL check on the context after a successful call to > > security_sid_to_context(). This is on the one hand redundant after > > checking for success and on the other hand insufficient on an actual > > NULL pointer, since the context is passed to seq_escape() leading to a > > call of strlen() on it. > > > > Reported by Clang analyzer: > > > > In file included from security/selinux/hooks.c:28: > > In file included from ./include/linux/tracehook.h:50: > > In file included from ./include/linux/memcontrol.h:13: > > In file included from ./include/linux/cgroup.h:18: > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > > seq_escape_mem(m, src, strlen(src), flags, esc); > > ^~~~~~~~~~~ > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/hooks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > I was waiting for Nick to reply, but he never did, and this looks good > to me so I just merged it into selinux/next. Thanks for your patience > Christian. LGTM; you can ping me on irc #ndesaulniers on most kernel channels if you're waiting on me. ;) > > -- > paul-moore.com -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] selinux: drop unnecessary NULL check 2022-06-07 21:26 ` Nick Desaulniers @ 2022-06-07 21:35 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-06-07 21:35 UTC (permalink / raw) To: Nick Desaulniers Cc: Christian Göttsche, selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Ondrej Mosnacek, Serge Hallyn, Austin Kim, Jiapeng Chong, Casey Schaufler, Yang Li, linux-kernel, llvm On Tue, Jun 7, 2022 at 5:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") > > > introduced a NULL check on the context after a successful call to > > > security_sid_to_context(). This is on the one hand redundant after > > > checking for success and on the other hand insufficient on an actual > > > NULL pointer, since the context is passed to seq_escape() leading to a > > > call of strlen() on it. > > > > > > Reported by Clang analyzer: > > > > > > In file included from security/selinux/hooks.c:28: > > > In file included from ./include/linux/tracehook.h:50: > > > In file included from ./include/linux/memcontrol.h:13: > > > In file included from ./include/linux/cgroup.h:18: > > > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] > > > seq_escape_mem(m, src, strlen(src), flags, esc); > > > ^~~~~~~~~~~ > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > --- > > > security/selinux/hooks.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > I was waiting for Nick to reply, but he never did, and this looks good > > to me so I just merged it into selinux/next. Thanks for your patience > > Christian. > > LGTM; you can ping me on irc #ndesaulniers on most kernel channels if > you're waiting on me. ;) Thanks, but I generally don't have the spare cycles to keep track of everyone's prefered method of interaction, that's why we've got the mailing list (warts and all) :) For what it's worth, I was waiting on you because you asked about the additional trace info and without any context I thought you might be looking for something else (?). In the end, I think everyone agreed that the patch was good so I merged it. I think as a general rule it's a good practice to follow-up with a reply when people provide additional information that you've requested. Not only is it the polite thing to do, it helps clarify things with everyone else that there is no hidden "gotcha!" in the patch. Regardless, thanks for checking back on this :) -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/5] selinux: drop return statement at end of void functions 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche ` (2 preceding siblings ...) 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche @ 2022-02-17 14:21 ` Christian Göttsche 2022-02-18 15:44 ` Paul Moore 2022-02-18 15:47 ` [PATCH 2/5] selinux: use correct type for context length Paul Moore 4 siblings, 1 reply; 24+ messages in thread From: Christian Göttsche @ 2022-02-17 14:21 UTC (permalink / raw) To: selinux Cc: Paul Moore, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Serge Hallyn, Yang Li, Austin Kim, Lakshmi Ramasubramanian, linux-kernel, llvm Those return statements at the end of a void function are redundant. Reported by clang-tidy [readability-redundant-control-flow] Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/hooks.c | 2 -- security/selinux/ss/conditional.c | 2 -- security/selinux/ss/ebitmap.c | 1 - security/selinux/ss/mls.c | 1 - security/selinux/ss/services.c | 2 -- 5 files changed, 8 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index dafabb4dcc64..1e69f88eb326 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3284,8 +3284,6 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, isec->sid = newsid; isec->initialized = LABEL_INITIALIZED; spin_unlock(&isec->lock); - - return; } static int selinux_inode_getxattr(struct dentry *dentry, const char *name) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 2ec6e5cd25d9..c46c419af512 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -566,8 +566,6 @@ void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key, if (node->key.specified & AVTAB_ENABLED) services_compute_xperms_decision(xpermd, node); } - return; - } /* Determine whether additional permissions are granted by the conditional * av table, and if so, add them to the result diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c index 61fcbb8d0f88..abde349c8321 100644 --- a/security/selinux/ss/ebitmap.c +++ b/security/selinux/ss/ebitmap.c @@ -359,7 +359,6 @@ void ebitmap_destroy(struct ebitmap *e) e->highbit = 0; e->node = NULL; - return; } int ebitmap_read(struct ebitmap *e, void *fp) diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c index 3f5fd124342c..99571b19d4a9 100644 --- a/security/selinux/ss/mls.c +++ b/security/selinux/ss/mls.c @@ -156,7 +156,6 @@ void mls_sid_to_context(struct policydb *p, } *scontext = scontextp; - return; } int mls_level_isvalid(struct policydb *p, struct mls_level *l) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 2f8db93e53b2..6901dc07680d 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -529,8 +529,6 @@ static void security_dump_masked_av(struct policydb *policydb, /* release scontext/tcontext */ kfree(tcontext_name); kfree(scontext_name); - - return; } /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] selinux: drop return statement at end of void functions 2022-02-17 14:21 ` [PATCH 1/5] selinux: drop return statement at end of void functions Christian Göttsche @ 2022-02-18 15:44 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-02-18 15:44 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Serge Hallyn, Yang Li, Austin Kim, Lakshmi Ramasubramanian, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Those return statements at the end of a void function are redundant. > > Reported by clang-tidy [readability-redundant-control-flow] > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 2 -- > security/selinux/ss/conditional.c | 2 -- > security/selinux/ss/ebitmap.c | 1 - > security/selinux/ss/mls.c | 1 - > security/selinux/ss/services.c | 2 -- > 5 files changed, 8 deletions(-) Merged into selinux/next, thanks. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] selinux: use correct type for context length 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche ` (3 preceding siblings ...) 2022-02-17 14:21 ` [PATCH 1/5] selinux: drop return statement at end of void functions Christian Göttsche @ 2022-02-18 15:47 ` Paul Moore 4 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2022-02-18 15:47 UTC (permalink / raw) To: Christian Göttsche Cc: selinux, Stephen Smalley, Eric Paris, Nathan Chancellor, Nick Desaulniers, Ondrej Mosnacek, Austin Kim, Kees Cook, Yang Li, Casey Schaufler, linux-kernel, llvm On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > security_sid_to_context() expects a pointer to an u32 as the address > where to store the length of the computed context. > > Reported by sparse: > > security/selinux/xfrm.c:359:39: warning: incorrect type in argument 4 (different signedness) > security/selinux/xfrm.c:359:39: expected unsigned int [usertype] *scontext_len > security/selinux/xfrm.c:359:39: got int * > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/xfrm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged into selinux/next, thanks. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-06-07 21:35 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche 2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche 2022-02-18 16:01 ` Paul Moore 2022-03-08 15:57 ` Christian Göttsche 2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche 2022-02-18 16:13 ` Paul Moore 2022-02-18 17:24 ` Nick Desaulniers 2022-02-22 23:16 ` Paul Moore 2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche 2022-04-04 20:03 ` Paul Moore 2022-05-02 14:43 ` [PATCH v3] " Christian Göttsche 2022-05-03 19:59 ` Paul Moore 2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche 2022-02-18 16:22 ` Paul Moore 2022-02-18 17:31 ` Nick Desaulniers 2022-03-08 16:09 ` Christian Göttsche 2022-05-02 13:43 ` Christian Göttsche 2022-05-04 11:15 ` Ondrej Mosnacek 2022-06-07 21:22 ` Paul Moore 2022-06-07 21:26 ` Nick Desaulniers 2022-06-07 21:35 ` Paul Moore 2022-02-17 14:21 ` [PATCH 1/5] selinux: drop return statement at end of void functions Christian Göttsche 2022-02-18 15:44 ` Paul Moore 2022-02-18 15:47 ` [PATCH 2/5] selinux: use correct type for context length Paul Moore
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.