| Message ID | 20240524093015.2402952-3-ivanov.mikhail1@huawei-partners.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Socket type control for Landlock | expand |
Hello Mikhail! Thanks for sending another revision of this patch set! On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote: > Add hook to security_socket_post_create(), which checks whether the socket > type and family are allowed by domain. Hook is called after initializing > the socket in the network stack to not wrongfully return EACCES for a > family-type pair, which is considered invalid by the protocol. > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> ## Some observations that *do not* need to be addressed in this commit, IMHO: get_raw_handled_socket_accesses, get_current_socket_domain and current_check_access_socket are based on the similarly-named functions from net.c (and fs.c), and it makes sense to stay consistent with these. There are some possible refactorings that could maybe be applied to that code, but given that the same ones would apply to net.c as well, it's probably best to address these separately. * Should get_raw_handled_socket_accesses be inlined? * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code? * Can we refactor out commonalities (probably not worth it right now though)? ## The only actionable feedback that I have that is specific to this commit is: In the past, we have introduced new (non-test) Landlock functionality in a single commit -- that way, we have no "loose ends" in the code between these two commits, and that simplifies it for people who want to patch your feature onto other kernel trees. (e.g. I think we should maybe merge commit 01/12 and 02/12 into a single commit.) WDYT? —Günther
5/27/2024 11:48 AM, Günther Noack wrote: > Hello Mikhail! > > Thanks for sending another revision of this patch set! > > On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote: >> Add hook to security_socket_post_create(), which checks whether the socket >> type and family are allowed by domain. Hook is called after initializing >> the socket in the network stack to not wrongfully return EACCES for a >> family-type pair, which is considered invalid by the protocol. >> >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > ## Some observations that *do not* need to be addressed in this commit, IMHO: > > get_raw_handled_socket_accesses, get_current_socket_domain and > current_check_access_socket are based on the similarly-named functions from > net.c (and fs.c), and it makes sense to stay consistent with these. > > There are some possible refactorings that could maybe be applied to that code, > but given that the same ones would apply to net.c as well, it's probably best to > address these separately. > > * Should get_raw_handled_socket_accesses be inlined It's a fairly simple and compact function, so compiler should inline it without any problems. Mickaël was against optional inlines [1]. [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/ > * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code? Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1 (see get_*_domain functions). > * Can we refactor out commonalities (probably not worth it right now though)? I had a few ideas about refactoring commonalities, as currently landlock has several repetitive patterns in the code. But solution requires a good design and a separate patch. Probably it's worth opening an issue on github. WDYT? > > > ## The only actionable feedback that I have that is specific to this commit is: > > In the past, we have introduced new (non-test) Landlock functionality in a > single commit -- that way, we have no "loose ends" in the code between these two > commits, and that simplifies it for people who want to patch your feature onto > other kernel trees. (e.g. I think we should maybe merge commit 01/12 and 02/12 > into a single commit.) WDYT? Yeah, this two should be merged and tests commits as well. I just wanted to do this in one of the latest patch versions to simplify code review. > > —Günther
Hello! On Thu, May 30, 2024 at 03:20:21PM +0300, Mikhail Ivanov wrote: > 5/27/2024 11:48 AM, Günther Noack wrote: > > On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote: > > > Add hook to security_socket_post_create(), which checks whether the socket > > > type and family are allowed by domain. Hook is called after initializing > > > the socket in the network stack to not wrongfully return EACCES for a > > > family-type pair, which is considered invalid by the protocol. > > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > > ## Some observations that *do not* need to be addressed in this commit, IMHO: > > > > get_raw_handled_socket_accesses, get_current_socket_domain and > > current_check_access_socket are based on the similarly-named functions from > > net.c (and fs.c), and it makes sense to stay consistent with these. > > > > There are some possible refactorings that could maybe be applied to that code, > > but given that the same ones would apply to net.c as well, it's probably best to > > address these separately. > > > > * Should get_raw_handled_socket_accesses be inlined > It's a fairly simple and compact function, so compiler should inline it > without any problems. Mickaël was against optional inlines [1]. > > [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/ Sorry for the confusion -- what I meant was not "should we add the inline keyword", but I meant "should we remove that function and place its implementation in the place where we are currently calling it"? > > * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code? > > Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1 > (see get_*_domain functions). What I found irritating about it is that with 0 layers (= no Landlock policy was ever enabled), you would logically assume that we return a success? But then I realized that this code was copied verbatim from other places in fs.c and net.c, and it is actually checking for an internal inconsistency that is never supposed to happen. If we were to actually hit that case at some point, we have probably stumbled over our own feet and it might be better to not permit anything. > > * Can we refactor out commonalities (probably not worth it right now though)? > > I had a few ideas about refactoring commonalities, as currently landlock > has several repetitive patterns in the code. But solution requires a > good design and a separate patch. Probably it's worth opening an issue > on github. WDYT? Absolutely, please do open one. In my mind, patches in C which might not get accepted are an expensive way to iterate on such ideas, and it might make sense to collect some refactoring approaches on a bug or the mailing list before jumping into the implementation. (You might want to keep an eye on https://github.com/landlock-lsm/linux/issues/1 as well, which is about some ideas to refactor Landlock's internal data structures.) > > ## The only actionable feedback that I have that is specific to this commit is: > > > > In the past, we have introduced new (non-test) Landlock functionality in a > > single commit -- that way, we have no "loose ends" in the code between these two > > commits, and that simplifies it for people who want to patch your feature onto > > other kernel trees. (e.g. I think we should maybe merge commit 01/12 and 02/12 > > into a single commit.) WDYT? > > Yeah, this two should be merged and tests commits as well. I just wanted > to do this in one of the latest patch versions to simplify code review. That sounds good, thanks! —Günther
6/5/2024 8:27 PM, Günther Noack wrote: > Hello! > > On Thu, May 30, 2024 at 03:20:21PM +0300, Mikhail Ivanov wrote: >> 5/27/2024 11:48 AM, Günther Noack wrote: >>> On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote: >>>> Add hook to security_socket_post_create(), which checks whether the socket >>>> type and family are allowed by domain. Hook is called after initializing >>>> the socket in the network stack to not wrongfully return EACCES for a >>>> family-type pair, which is considered invalid by the protocol. >>>> >>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >>> >>> ## Some observations that *do not* need to be addressed in this commit, IMHO: >>> >>> get_raw_handled_socket_accesses, get_current_socket_domain and >>> current_check_access_socket are based on the similarly-named functions from >>> net.c (and fs.c), and it makes sense to stay consistent with these. >>> >>> There are some possible refactorings that could maybe be applied to that code, >>> but given that the same ones would apply to net.c as well, it's probably best to >>> address these separately. >>> >>> * Should get_raw_handled_socket_accesses be inlined >> It's a fairly simple and compact function, so compiler should inline it >> without any problems. Mickaël was against optional inlines [1]. >> >> [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/ > > Sorry for the confusion -- what I meant was not "should we add the inline > keyword", but I meant "should we remove that function and place its > implementation in the place where we are currently calling it"? Oh, I got it, thanks! It will be great to find a way how to generalize this helpers. But if we won't come up with some good design, it will be really better to simply inline them. I added a mark about this in code refactoring issue [1]. [1] https://github.com/landlock-lsm/linux/issues/34 > > >>> * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code? >> >> Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1 >> (see get_*_domain functions). > > What I found irritating about it is that with 0 layers (= no Landlock policy was > ever enabled), you would logically assume that we return a success? But then I > realized that this code was copied verbatim from other places in fs.c and net.c, > and it is actually checking for an internal inconsistency that is never supposed > to happen. If we were to actually hit that case at some point, we have probably > stumbled over our own feet and it might be better to not permit anything. This check is probably really useful for validating code changes. > > >>> * Can we refactor out commonalities (probably not worth it right now though)? >> >> I had a few ideas about refactoring commonalities, as currently landlock >> has several repetitive patterns in the code. But solution requires a >> good design and a separate patch. Probably it's worth opening an issue >> on github. WDYT? > > Absolutely, please do open one. In my mind, patches in C which might not get > accepted are an expensive way to iterate on such ideas, and it might make sense > to collect some refactoring approaches on a bug or the mailing list before > jumping into the implementation. > > (You might want to keep an eye on https://github.com/landlock-lsm/linux/issues/1 > as well, which is about some ideas to refactor Landlock's internal data > structures.) Thank you! Discussing refactoring ideas before actually implementing them sounds really great. We can collect multiple ideas, discuss them and implement a single dedicated patchlist. Issue: https://github.com/landlock-lsm/linux/issues/34. > > >>> ## The only actionable feedback that I have that is specific to this commit is: >>> >>> In the past, we have introduced new (non-test) Landlock functionality in a >>> single commit -- that way, we have no "loose ends" in the code between these two >>> commits, and that simplifies it for people who want to patch your feature onto >>> other kernel trees. (e.g. I think we should maybe merge commit 01/12 and 02/12 >>> into a single commit.) WDYT? >> >> Yeah, this two should be merged and tests commits as well. I just wanted >> to do this in one of the latest patch versions to simplify code review. > > That sounds good, thanks! > > —Günther
diff --git a/security/landlock/setup.c b/security/landlock/setup.c index 28519a45b11f..fd4e7e8f3cb2 100644 --- a/security/landlock/setup.c +++ b/security/landlock/setup.c @@ -14,6 +14,7 @@ #include "cred.h" #include "fs.h" #include "net.h" +#include "socket.h" #include "setup.h" #include "task.h" @@ -37,6 +38,7 @@ static int __init landlock_init(void) landlock_add_task_hooks(); landlock_add_fs_hooks(); landlock_add_net_hooks(); + landlock_add_socket_hooks(); landlock_initialized = true; pr_info("Up and running.\n"); return 0; diff --git a/security/landlock/socket.c b/security/landlock/socket.c index 1249a4a36503..b2775473b3dc 100644 --- a/security/landlock/socket.c +++ b/security/landlock/socket.c @@ -8,7 +8,9 @@ #include <linux/net.h> #include <linux/socket.h> #include <linux/stddef.h> +#include <net/ipv6.h> +#include "cred.h" #include "limits.h" #include "ruleset.h" #include "socket.h" @@ -58,3 +60,71 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset, return err; } + +static access_mask_t +get_raw_handled_socket_accesses(const struct landlock_ruleset *const domain) +{ + access_mask_t access_dom = 0; + size_t layer_level; + + for (layer_level = 0; layer_level < domain->num_layers; layer_level++) + access_dom |= + landlock_get_socket_access_mask(domain, layer_level); + return access_dom; +} + +static const struct landlock_ruleset *get_current_socket_domain(void) +{ + const struct landlock_ruleset *const dom = + landlock_get_current_domain(); + + if (!dom || !get_raw_handled_socket_accesses(dom)) + return NULL; + + return dom; +} + +static int current_check_access_socket(struct socket *const sock, int family, + int type, + const access_mask_t access_request) +{ + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {}; + const struct landlock_rule *rule; + access_mask_t handled_access; + struct landlock_id id = { + .type = LANDLOCK_KEY_SOCKET, + }; + const struct landlock_ruleset *const dom = get_current_socket_domain(); + + if (!dom) + return 0; + if (WARN_ON_ONCE(dom->num_layers < 1)) + return -EACCES; + + id.key.data = pack_socket_key(family, type); + + rule = landlock_find_rule(dom, id); + handled_access = landlock_init_layer_masks( + dom, access_request, &layer_masks, LANDLOCK_KEY_SOCKET); + if (landlock_unmask_layers(rule, handled_access, &layer_masks, + ARRAY_SIZE(layer_masks))) + return 0; + return -EACCES; +} + +static int hook_socket_create(struct socket *const sock, int family, int type, + int protocol, int kern) +{ + return current_check_access_socket(sock, family, type, + LANDLOCK_ACCESS_SOCKET_CREATE); +} + +static struct security_hook_list landlock_hooks[] __ro_after_init = { + LSM_HOOK_INIT(socket_post_create, hook_socket_create), +}; + +__init void landlock_add_socket_hooks(void) +{ + security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks), + &landlock_lsmid); +} diff --git a/security/landlock/socket.h b/security/landlock/socket.h index 8519357f1c39..5c36eae9732f 100644 --- a/security/landlock/socket.h +++ b/security/landlock/socket.h @@ -10,6 +10,8 @@ #include "ruleset.h" +__init void landlock_add_socket_hooks(void); + int landlock_append_socket_rule(struct landlock_ruleset *const ruleset, const int family, const int type, access_mask_t access_rights);
Add hook to security_socket_post_create(), which checks whether the socket type and family are allowed by domain. Hook is called after initializing the socket in the network stack to not wrongfully return EACCES for a family-type pair, which is considered invalid by the protocol. Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> --- Changes since v1: * Use lsm hook arguments instead of struct socket fields as family-type values. * Packs socket family and type using helper. * Fixes commit message. * Formats with clang-format. --- security/landlock/setup.c | 2 ++ security/landlock/socket.c | 70 ++++++++++++++++++++++++++++++++++++++ security/landlock/socket.h | 2 ++ 3 files changed, 74 insertions(+)