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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
>
> ---
> 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
On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> security/selinux/xfrm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into selinux/next, thanks.
--
paul-moore.com
On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> <[email protected]> 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
On Fri, Feb 18, 2022 at 12:24 PM Nick Desaulniers
<[email protected]> wrote:
> On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <[email protected]> wrote:
> > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> > <[email protected]> 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
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 <[email protected]>
---
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
On Fri, 18 Feb 2022 at 17:01, Paul Moore <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> <[email protected]> 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 <[email protected]>
> >
> > ---
> > 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
On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> <[email protected]> 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 <[email protected]>
> > ---
> > 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);
^ ~~~
On Tue, Mar 8, 2022 at 11:55 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <[email protected]> wrote:
>
> On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <[email protected]> wrote:
> >
> > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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);
> ^ ~~~
On Mon, May 2, 2022 at 10:43 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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
On Mon, May 2, 2022 at 3:43 PM Christian Göttsche
<[email protected]> wrote:
>
> On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <[email protected]> wrote:
> >
> > On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <[email protected]> wrote:
> > >
> > > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Jun 7, 2022 at 5:26 PM Nick Desaulniers <[email protected]> wrote:
>
> On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <[email protected]> wrote:
> >
> > On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<[email protected]> 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 <[email protected]>
> ---
> 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