Check if the registering LSM already registered hooks just before. This
enable to split hook declarations into multiple files without
registering multiple time the same LSM name, starting from commit
d69dece5f5b6 ("LSM: Add /sys/kernel/security/lsm").
Signed-off-by: Mickaël Salaün <[email protected]>
Cc: Casey Schaufler <[email protected]>
Cc: James Morris <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Serge E. Hallyn <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
security/security.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/security/security.c b/security/security.c
index 549bddcc2116..6be65050b268 100644
--- a/security/security.c
+++ b/security/security.c
@@ -25,6 +25,7 @@
#include <linux/mount.h>
#include <linux/personality.h>
#include <linux/backing-dev.h>
+#include <linux/string.h>
#include <net/flow.h>
#define MAX_LSM_EVM_XATTR 2
@@ -86,6 +87,32 @@ static int __init choose_lsm(char *str)
}
__setup("security=", choose_lsm);
+static bool match_last_lsm(const char *list, const char *last)
+{
+ size_t list_len, last_len, i;
+
+ if (!list || !last)
+ return false;
+ list_len = strlen(list);
+ last_len = strlen(last);
+ if (!last_len || !list_len)
+ return false;
+ if (last_len > list_len)
+ return false;
+
+ for (i = 0; i < last_len; i++) {
+ if (list[list_len - 1 - i] != last[last_len - 1 - i])
+ return false;
+ }
+ /* Check if last_len == list_len */
+ if (i == list_len)
+ return true;
+ /* Check if it is a full name */
+ if (list[list_len - 1 - i] == ',')
+ return true;
+ return false;
+}
+
static int lsm_append(char *new, char **result)
{
char *cp;
@@ -93,6 +120,9 @@ static int lsm_append(char *new, char **result)
if (*result == NULL) {
*result = kstrdup(new, GFP_KERNEL);
} else {
+ /* Check if it is the last registered name */
+ if (match_last_lsm(*result, new))
+ return 0;
cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new);
if (cp == NULL)
return -ENOMEM;
--
2.11.0
On 4/29/2017 12:02 PM, Mickaël Salaün wrote:
> Check if the registering LSM already registered hooks just before. This
> enable to split hook declarations into multiple files without
> registering multiple time the same LSM name, starting from commit
> d69dece5f5b6 ("LSM: Add /sys/kernel/security/lsm").
What's special about the previous registration? Keep it
simple and check it the name is already anywhere on the
list and only add it if it's not already there. I don't
see advantage to:
% cat /sys/kernel/security/lsm
capability,yama,spiffy,selinux,spiffy
over
% cat /sys/kernel/security/lsm
capability,yama,spiffy,selinux
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Serge E. Hallyn <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> security/security.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index 549bddcc2116..6be65050b268 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -25,6 +25,7 @@
> #include <linux/mount.h>
> #include <linux/personality.h>
> #include <linux/backing-dev.h>
> +#include <linux/string.h>
> #include <net/flow.h>
>
> #define MAX_LSM_EVM_XATTR 2
> @@ -86,6 +87,32 @@ static int __init choose_lsm(char *str)
> }
> __setup("security=", choose_lsm);
>
> +static bool match_last_lsm(const char *list, const char *last)
> +{
> + size_t list_len, last_len, i;
> +
> + if (!list || !last)
> + return false;
> + list_len = strlen(list);
> + last_len = strlen(last);
> + if (!last_len || !list_len)
> + return false;
> + if (last_len > list_len)
> + return false;
> +
> + for (i = 0; i < last_len; i++) {
> + if (list[list_len - 1 - i] != last[last_len - 1 - i])
> + return false;
> + }
> + /* Check if last_len == list_len */
> + if (i == list_len)
> + return true;
> + /* Check if it is a full name */
> + if (list[list_len - 1 - i] == ',')
> + return true;
> + return false;
> +}
> +
> static int lsm_append(char *new, char **result)
> {
> char *cp;
> @@ -93,6 +120,9 @@ static int lsm_append(char *new, char **result)
> if (*result == NULL) {
> *result = kstrdup(new, GFP_KERNEL);
> } else {
> + /* Check if it is the last registered name */
> + if (match_last_lsm(*result, new))
> + return 0;
> cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new);
> if (cp == NULL)
> return -ENOMEM;
Casey Schaufler wrote:
> On 4/29/2017 12:02 PM, Mickael Salaun wrote:
> > Check if the registering LSM already registered hooks just before. This
> > enable to split hook declarations into multiple files without
> > registering multiple time the same LSM name, starting from commit
> > d69dece5f5b6 ("LSM: Add /sys/kernel/security/lsm").
>
> What's special about the previous registration? Keep it
> simple and check it the name is already anywhere on the
> list and only add it if it's not already there. I don't
> see advantage to:
>
> % cat /sys/kernel/security/lsm
> capability,yama,spiffy,selinux,spiffy
>
> over
> % cat /sys/kernel/security/lsm
> capability,yama,spiffy,selinux
>
- if (lsm_append(lsm, &lsm_names) < 0)
+ if (lsm && lsm_append(lsm, &lsm_names) < 0)
in security_add_hooks()?
On 30/04/2017 04:11, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 4/29/2017 12:02 PM, Mickael Salaun wrote:
>>> Check if the registering LSM already registered hooks just before. This
>>> enable to split hook declarations into multiple files without
>>> registering multiple time the same LSM name, starting from commit
>>> d69dece5f5b6 ("LSM: Add /sys/kernel/security/lsm").
>>
>> What's special about the previous registration? Keep it
>> simple and check it the name is already anywhere on the
>> list and only add it if it's not already there. I don't
>> see advantage to:
>>
>> % cat /sys/kernel/security/lsm
>> capability,yama,spiffy,selinux,spiffy
>>
>> over
>> % cat /sys/kernel/security/lsm
>> capability,yama,spiffy,selinux
>>
That was my first though, but then I realized that I don't see any use
case where an LSM would register hooks interleaved with other LSM. I
find the current approach simpler because we only search from the end of
the string and we do not handle special cases (e.g. matching only a
sub-name). Moreover, this approach respects the semantic describe in
Documentation/security/LSM.txt: "The list reflects the order in which
checks are made".
>
> - if (lsm_append(lsm, &lsm_names) < 0)
> + if (lsm && lsm_append(lsm, &lsm_names) < 0)
>
> in security_add_hooks()?
>
That was considered
[https://lkml.kernel.org/r/CAGXu5jJCvJ6-uZ=Kfhh3xD7UvaY+G99e9NXFMzvi=9OQzA6Ecg@mail.gmail.com]
but Kees and Casey seem to prefer the current approach.
On Sat, 29 Apr 2017, Mickaël Salaün wrote:
> Check if the registering LSM already registered hooks just before. This
> enable to split hook declarations into multiple files without
> registering multiple time the same LSM name, starting from commit
> d69dece5f5b6 ("LSM: Add /sys/kernel/security/lsm").
Please include a detailed rationale for these patches. The above tells us
very little about why they are needed.
--
James Morris
<[email protected]>