On 30/12/2021 02:43, Konstantin Meskhidze wrote:
Hi Mickaël,
Thanks for the quick reply.
I apologise that I did not follow some rules here.
I'm a newbie in the Linux community communication and probably missed
some important points.
Thank you for noticing about them - I will fix my misses.
No worries, these RFCs are here to improve the proposition but also to
learn.
12/29/2021 1:09 AM, Mickaël Salaün wrote:
Hi Konstantin,
Please read the full how-to about sending patches:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
There are at least these issues:
- no link to the previous version:
https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@xxxxxxxxxx/
- no version: [RFC PATCH v2 0/1]
- even if there is only one patch, please make the cover letter a
separate email (i.e. git format-patch --cover-letter).
I got it. My mistake.
Anyway I can resend the patch ,if you would like, with all corrections.
No need to resend the same patch series, just follow these rules for the
next patch series.
It seems that you missed some of my previous (inlined) comments, you
didn't reply to most of them:
https://lore.kernel.org/linux-security-module/b50ed53a-683e-77cf-9dc2-f4ae1b5fa0fd@xxxxxxxxxxx/
Sorry about that. I will take it into account.
I will reply your previous comments.
Did you test your changes before submitting this patch series?
I tested it into QEMU environment. I wrote net_sandboxer.c
file just for test insering network rules and bind() and connect()
syscalls.
Ok, but I'm wondering about the content if this code and what you
actually tested with it. This is one reason why included standalone
tests are useful.
Also I launched your sandboxer.c and it worked well.
I'm going to provide seltests in another patch series.
Great!
This series can only forbids TCP connects and binds to processes other
than the one creating the ruleset, which doesn't make sense here (as I
already explained). TCP ports are not checked.
Yes. I provided just inserting network access rules into a prosess
ruleset.For the port checking, I wanted to ask your opinion about how it
would possible to insert port rule. Cause I have my view, that differs
from your one.
If you have any doubts about any of the comments, please rephrase,
challenge them and ask questions.
On 28/12/2021 12:52, Konstantin Meskhidze wrote:
Hi, all!
Here is another PoC patch for Landlock network confinement.
Now 2 hooks are supported for TCP sockets:
- hook_socket_bind()
- hook_socket_connect()
After architectuire has been more clear, there will be a patch with
selftests.
Please welcome with any comments and suggestions.
Implementation related issues
=============================
1. It was suggested by Mickaёl using new network rules
attributes, like:
struct landlock_net_service_attr {
__u64 allowed_access; // LANDLOCK_NET_*_TCP
__u16 port;
} __attribute__((packed));
I found that, if we want to support inserting port attributes,
it's needed to add port member into struct landlock_rule:
struct landlock_rule {
...
struct landlock_object *object;
/**
* @num_layers: Number of entries in @layers.
*/
u32 num_layers;
u16 port;
In this case the "object" is indeed defined by a port. You can then
create an union containing either a struct landlock_object pointer or
a raw value (here a u16 port). Of course, every code that use the
object field must be audited and updated accordingly. I think the
following update should be a good approach (with updated documentation):
struct landlock_rule {
[...]
union {
struct landlock_object *ptr;
uintptr_t data;
} object;
[...]
};
…and then a function helper to convert raw data to/from port.
It would be a good idea to use a dedicated tree for objects identified
by (typed) data vs. pointer:
struct landlock_ruleset {
struct rb_root root_inode; // i.e. the current "root" field.
struct rb_root root_net_port;
[...]
};
This approach makes sense. I did not think in this way. I was following
the concept that every rule must be tied to a real kernel object. Using
pseudo "object" defined by a port is a good idea. Thanks.
...
};
In this case 2 functions landlock_insert_rule() and insert_rule()
must be refactored;
But, if struct landlock_layer be modified -
struct landlock_layer {
/**
* @level: Position of this layer in the layer stack.
*/
u16 level;
/**
* @access: Bitfield of allowed actions on the kernel object.
They are
* relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ).
*/
u16 access;
u16 port;
No, struct landlock_layer doesn't need to be modified. This struct is
independent from the type of object. I talked about that in the
previous series.
I got it. Thanks for the comment.
};
so, just one landlock_insert_rule() must be slightly refactored.
Also many new attributes could be easily supported in future versions.
2. access_masks[] member of struct landlock_ruleset was modified
to support multiple rule type masks.
I suggest using 2D array semantic for convenient usage:
access_masks[rule_type][layer_level]
But its also possible to use 1D array with modulo arithmetic:
access_masks[rule_type % layer_level]
What about my previous suggestion to use a (well defined) upper bit to
identify the type of access?
Maybe I missed your point here. But I can't see how to identify
different rule types here. If there just one ruleset created, so there
is just one access_mask[0] in the ruleset. Its either can be used for
filesystem mask or for network one. To support both rule type we need to
use at least access_mask[] array with size of 2.
Also using upper bit to identify access rule type will narrow down
possible access ammount in future version.
Anyway please corrent me if I'm wrong here.
We should avoid using multi-dimentional arrays because it makes the code
more complex (e.g. when allocating memory).
The idea is to split the access masks' u16 to the actual access masks
and a bit to identify the type of access. Because there is no more than
13 access rights for now (per access type, i.e. for the file system),
the 3 upper bits are unused. A simple bit mask (and a shift) can get
these 3 upper bits or the 13 lower bits. If/when new access bits will be
used, we'll just need to upgrade the u16 to a u32, but the logic will be
the same and the userspace ABI unchanged.
[...]
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index ec72b9262bf3..a335c475965c 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -27,9 +27,24 @@
static struct landlock_ruleset *create_ruleset(const u32 num_layers)
{
struct landlock_ruleset *new_ruleset;
+ u16 row, col, rules_types_num;
+
+ new_ruleset = kzalloc(sizeof *new_ruleset +
+ sizeof *(new_ruleset->access_masks),
sizeof(access_masks) is 0.
Actually sizeof *(new_ruleset->access_masks) is 8.
It's a 64 bit pointer to u16 array[]. I checked this
2D FAM array implementation in a standalone test.
Yes, this gives the size of the pointed element, but I wanted to point
out that access_masks doesn't have a size (actually a sizeof() call on
it would failed). This kzalloc() only allocates one element in the
array. What happen when there is more than one layer?
+ GFP_KERNEL_ACCOUNT);
+
+ rules_types_num = LANDLOCK_RULE_TYPE_NUM;
+ /* Initializes access_mask array for multiple rule types.
+ * Double array semantic is used convenience:
access_mask[rule_type][num_layer].
+ */
+ for (row = 0; row < rules_types_num; row++) {
+ new_ruleset->access_masks[row] = kzalloc(sizeof
+ *(new_ruleset->access_masks[row]),
+ GFP_KERNEL_ACCOUNT);
+ for (col = 0; col < num_layers; col++)
+ new_ruleset->access_masks[row][col] = 0;
+ }
This code may segfault. I guess it wasn't tested. Please enable most
test/check features as I suggested in the previous series.
I compiled the kernel 5.13 with this patch and tested it in QEMU. No
crashes. I tested your sandboxer.c and it works without segfaults.
But I will provide seltests in future patch.
Did you run the current selftests?
fakeroot make -C tools/testing/selftests TARGETS=landlock gen_tar
tar -xf
tools/testing/selftests/kselftest_install/kselftest-packages/kselftest.tar.gz
# as root in a VM with Landlock enabled:
./run_kselftest.sh
BTW, you should test with the latest kernel (i.e. latest Linus's tag).
- new_ruleset = kzalloc(struct_size(new_ruleset, fs_access_masks,
- num_layers), GFP_KERNEL_ACCOUNT);
What about my comment in the previous series?
As I replied above.
Maybe I missed your point here. But I can't see how to identify
different rule types here. If there just one ruleset created, so there
is just one access_mask[0] in the ruleset. Its either can be used for
filesystem mask or for network one. To support both rule type we need to
use at least access_mask[] array with size of 2.
Also using upper bit to identify access rule type will narrow down
possible access ammount in future version.
I suggest using 2D array or 1D array with module arithmetic to
support different rule type masks.
Anyway please corrent me if I'm wrong here.
See my above comment.
[...]
