Re: [PATCH 0/2] landlock: truncate(2) support

Re: [PATCH 0/2] landlock: truncate(2) support

[Mickaël Salaün]

Hi Günther, this looks good!

Added linux-fsdevel@vger.kernel.org

On 07/07/2022 22:06, Günther Noack wrote:
> The goal of these patches is to work towards a more complete coverage
> of file system operations that are restrictable with Landlock.
> 
> The known set of currently unsupported file system operations in
> Landlock is described at [1]. Out of the operations listed there,
> truncate is the only one that modifies file contents, so these patches
> should make it possible to prevent the direct modification of file
> contents with Landlock.
> 
> The patch introduces the truncate(2) restriction feature as an
> additional bit in the access_mask_t bitmap, in line with the existing
> supported operations.
> 
> Apart from Landlock, the truncate(2) and ftruncate(2) family of system
> calls can also be restricted using seccomp-bpf, but it is a
> complicated mechanism (requires BPF, requires keeping up-to-date
> syscall lists) and it also is not configurable by file hierarchy, as
> Landlock is. The simplicity and flexibility of the Landlock approach
> makes it worthwhile adding.
> 
> I am aware that the documentation and samples/landlock/sandboxer.c
> tool still need corresponding updates; I'm hoping to get some early
> feedback this way.
Yes, that's a good approach.

Extending the sandboxer should be straightforward, you can just extend 
the scope of LL_FS_RW, taking into account the system Landlock ABI 
because there is no "contract" for this sample.

You'll need to remove the warning about truncate(2) in the 
documentation, and maybe to move it to the "previous limitations" 
section, with the LANDLOCK_ACCESS_TRUNCATE doc pointing to it. I think 
it would be nice to extend the LANDLOCK_ACCESS_FS_WRITE documentation to 
point to LANDLOCK_ACCESS_FS_TRUNCATE because this distinction could be 
disturbing for users. Indeed, all inode-based LSMs (SELinux and Smack) 
deny such action if the inode is not writable (with the inode_permission 
check), which is not the case for path-based LSMs (AppArmor and Tomoyo).

While we may question whether a dedicated access right should be added 
for the Landlock use case, two arguments are in favor of this approach:
- For compatibility reasons, the kernel must follow the semantic of a 
specific Landlock ABI, otherwise it could break user space. We could 
still backport this patch and merge it with the ABI 1 and treat it as a 
bug, but the initial version of Landlock was meant to be an MVP, hence 
this lack of access right.
- There is a specific access right for Capsicum (CAP_FTRUNCATE) that 
could makes more sense in the future.

Following the Capsicum semantic, I think it would be a good idea to also 
check for the O_TRUNC open flag: 
https://www.freebsd.org/cgi/man.cgi?query=rights


> 
> These patches are based on version 5.19-rc5.
> The patch set can also be browsed on the web at [2].
> 
> Best regards,
> Günther
> 
> [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> [2] https://github.com/gnoack/linux/tree/landlock-truncate
> 
> Günther Noack (2):
>    landlock: Support truncate(2).
>    landlock: Selftests for truncate(2) support.
> 
>   include/uapi/linux/landlock.h                |  2 +
>   security/landlock/fs.c                       |  9 +-
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   | 87 +++++++++++++++++++-
>   6 files changed, 97 insertions(+), 7 deletions(-)
> 
> --
> 2.37.0

Re: [PATCH 0/2] landlock: truncate(2) support

[Günther Noack]

Hello Mickaël!

Thank you for the fast feedback! I'm looking into your comments from
this mail and the rest of the thread and am working on an updated
patch set.

On Fri, Jul 08, 2022 at 01:16:29PM +0200, Mickaël Salaün wrote:
> Hi Günther, this looks good!
>
> Added linux-fsdevel@vger.kernel.org
>
> On 07/07/2022 22:06, Günther Noack wrote:
> > The goal of these patches is to work towards a more complete coverage
> > of file system operations that are restrictable with Landlock.
> >
> > The known set of currently unsupported file system operations in
> > Landlock is described at [1]. Out of the operations listed there,
> > truncate is the only one that modifies file contents, so these patches
> > should make it possible to prevent the direct modification of file
> > contents with Landlock.
> >
> > The patch introduces the truncate(2) restriction feature as an
> > additional bit in the access_mask_t bitmap, in line with the existing
> > supported operations.
> >
> > Apart from Landlock, the truncate(2) and ftruncate(2) family of system
> > calls can also be restricted using seccomp-bpf, but it is a
> > complicated mechanism (requires BPF, requires keeping up-to-date
> > syscall lists) and it also is not configurable by file hierarchy, as
> > Landlock is. The simplicity and flexibility of the Landlock approach
> > makes it worthwhile adding.
> >
> > I am aware that the documentation and samples/landlock/sandboxer.c
> > tool still need corresponding updates; I'm hoping to get some early
> > feedback this way.
> Yes, that's a good approach.
>
> Extending the sandboxer should be straightforward, you can just extend the
> scope of LL_FS_RW, taking into account the system Landlock ABI because there
> is no "contract" for this sample.

Sounds good, I'll extend the sample tool like this for the updated patch set.

(On the side, as you know from the discussion on the go-landlock
library, I have some suspicion that the "best effort"
backwards-compatibility approach in the sample tool is not the right
one for the "refer" right, but that might be better suited for a
separate patch. Maybe it'll be simpler to just not support a
best-effort downgrade in the sample tool.)

> You'll need to remove the warning about truncate(2) in the documentation,
> and maybe to move it to the "previous limitations" section, with the
> LANDLOCK_ACCESS_TRUNCATE doc pointing to it. I think it would be nice to
> extend the LANDLOCK_ACCESS_FS_WRITE documentation to point to
> LANDLOCK_ACCESS_FS_TRUNCATE because this distinction could be disturbing for
> users. Indeed, all inode-based LSMs (SELinux and Smack) deny such action if
> the inode is not writable (with the inode_permission check), which is not
> the case for path-based LSMs (AppArmor and Tomoyo).

This makes a lot of sense, I'll work on the documentation to point this out.

I suspect that for many common use cases, the
LANDLOCK_ACCESS_FS_TRUNCATE right will anyway only be used together
with LANDLOCK_ACCESS_FS_FILE_WRITE in practice. (See below for more
detail.)

> While we may question whether a dedicated access right should be added for
> the Landlock use case, two arguments are in favor of this approach:
> - For compatibility reasons, the kernel must follow the semantic of a
> specific Landlock ABI, otherwise it could break user space. We could still
> backport this patch and merge it with the ABI 1 and treat it as a bug, but
> the initial version of Landlock was meant to be an MVP, hence this lack of
> access right.
> - There is a specific access right for Capsicum (CAP_FTRUNCATE) that could
> makes more sense in the future.
>
> Following the Capsicum semantic, I think it would be a good idea to also
> check for the O_TRUNC open flag:
> https://www.freebsd.org/cgi/man.cgi?query=rights

open() with O_TRUNC was indeed a case I had not thought about - thanks
for pointing it out.

I started adding some tests for it, and found to my surprise that
open() *is* already checking security_path_truncate() when it is
truncating files. So there is a chance that we can get away without a
special check for O_TRUNC in the security_file_open hook.

The exact semantics might be slightly different to Capsicum though -
in particular, the creat() call (= open with O_TRUNC|O_CREAT|O_WRONLY)
will require the Landlock truncate right when it's overwriting an
existing regular file, but it will not require the Landlock truncate
right when it's creating a new file.

I'm not fully sure how this is done in Capsicum. I assume that the
Comparison with Capsicum is mostly for inspiration, but there is no
goal of being fully compatible with that model?

The creat() behaviour is non-intuitive from userspace, I think:
creat() is a pretty common way to create new files, and it might come
as a surprise to people that this can require the truncate right,
because:

- The function creat() doesn't have "truncate" in its name, and you
  might be tempted to think that the LANDLOCK_ACCESS_FS_MAKE_REG is
  sufficient for calling it.

- Users can work around the need for the truncate right by unlinking
  the existing regular file with the same name and creating a new one.
  So for the most common use case (where users do not care about the
  file's inode identity or race conditions), it is surprising that
  the truncate right is required.

Summarizing this, I also think that the truncate right needs to be a
separate flag, even if just for backwards compatibility reasons.

But at the same time, I suspect that in practice, the truncate right
will probably have to usually go together with the file_write right,
so that the very common creat() use case (and possibly others) does
not yield surprising behaviour.

—Günther

>
>
> >
> > These patches are based on version 5.19-rc5.
> > The patch set can also be browsed on the web at [2].
> >
> > Best regards,
> > Günther
> >
> > [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> > [2] https://github.com/gnoack/linux/tree/landlock-truncate
> >
> > Günther Noack (2):
> >    landlock: Support truncate(2).
> >    landlock: Selftests for truncate(2) support.
> >
> >   include/uapi/linux/landlock.h                |  2 +
> >   security/landlock/fs.c                       |  9 +-
> >   security/landlock/limits.h                   |  2 +-
> >   security/landlock/syscalls.c                 |  2 +-
> >   tools/testing/selftests/landlock/base_test.c |  2 +-
> >   tools/testing/selftests/landlock/fs_test.c   | 87 +++++++++++++++++++-
> >   6 files changed, 97 insertions(+), 7 deletions(-)
> >
> > --
> > 2.37.0

--

Re: [PATCH 0/2] landlock: truncate(2) support

[Mickaël Salaün]

On 10/07/2022 11:57, Günther Noack wrote:
> Hello Mickaël!
> 
> Thank you for the fast feedback! I'm looking into your comments from
> this mail and the rest of the thread and am working on an updated
> patch set.
> 
> On Fri, Jul 08, 2022 at 01:16:29PM +0200, Mickaël Salaün wrote:
>> Hi Günther, this looks good!
>>
>> Added linux-fsdevel@vger.kernel.org
>>
>> On 07/07/2022 22:06, Günther Noack wrote:
>>> The goal of these patches is to work towards a more complete coverage
>>> of file system operations that are restrictable with Landlock.
>>>
>>> The known set of currently unsupported file system operations in
>>> Landlock is described at [1]. Out of the operations listed there,
>>> truncate is the only one that modifies file contents, so these patches
>>> should make it possible to prevent the direct modification of file
>>> contents with Landlock.
>>>
>>> The patch introduces the truncate(2) restriction feature as an
>>> additional bit in the access_mask_t bitmap, in line with the existing
>>> supported operations.
>>>
>>> Apart from Landlock, the truncate(2) and ftruncate(2) family of system
>>> calls can also be restricted using seccomp-bpf, but it is a
>>> complicated mechanism (requires BPF, requires keeping up-to-date
>>> syscall lists) and it also is not configurable by file hierarchy, as
>>> Landlock is. The simplicity and flexibility of the Landlock approach
>>> makes it worthwhile adding.
>>>
>>> I am aware that the documentation and samples/landlock/sandboxer.c
>>> tool still need corresponding updates; I'm hoping to get some early
>>> feedback this way.
>> Yes, that's a good approach.
>>
>> Extending the sandboxer should be straightforward, you can just extend the
>> scope of LL_FS_RW, taking into account the system Landlock ABI because there
>> is no "contract" for this sample.
> 
> Sounds good, I'll extend the sample tool like this for the updated patch set.
> 
> (On the side, as you know from the discussion on the go-landlock
> library, I have some suspicion that the "best effort"
> backwards-compatibility approach in the sample tool is not the right
> one for the "refer" right, but that might be better suited for a
> separate patch. Maybe it'll be simpler to just not support a
> best-effort downgrade in the sample tool.)

Please share your though about the "refer" right.


> 
>> You'll need to remove the warning about truncate(2) in the documentation,
>> and maybe to move it to the "previous limitations" section, with the
>> LANDLOCK_ACCESS_TRUNCATE doc pointing to it. I think it would be nice to
>> extend the LANDLOCK_ACCESS_FS_WRITE documentation to point to
>> LANDLOCK_ACCESS_FS_TRUNCATE because this distinction could be disturbing for
>> users. Indeed, all inode-based LSMs (SELinux and Smack) deny such action if
>> the inode is not writable (with the inode_permission check), which is not
>> the case for path-based LSMs (AppArmor and Tomoyo).
> 
> This makes a lot of sense, I'll work on the documentation to point this out.
> 
> I suspect that for many common use cases, the
> LANDLOCK_ACCESS_FS_TRUNCATE right will anyway only be used together
> with LANDLOCK_ACCESS_FS_FILE_WRITE in practice. (See below for more
> detail.)

Agree


> 
>> While we may question whether a dedicated access right should be added for
>> the Landlock use case, two arguments are in favor of this approach:
>> - For compatibility reasons, the kernel must follow the semantic of a
>> specific Landlock ABI, otherwise it could break user space. We could still
>> backport this patch and merge it with the ABI 1 and treat it as a bug, but
>> the initial version of Landlock was meant to be an MVP, hence this lack of
>> access right.
>> - There is a specific access right for Capsicum (CAP_FTRUNCATE) that could
>> makes more sense in the future.
>>
>> Following the Capsicum semantic, I think it would be a good idea to also
>> check for the O_TRUNC open flag:
>> https://www.freebsd.org/cgi/man.cgi?query=rights
> 
> open() with O_TRUNC was indeed a case I had not thought about - thanks
> for pointing it out.
> 
> I started adding some tests for it, and found to my surprise that
> open() *is* already checking security_path_truncate() when it is
> truncating files. So there is a chance that we can get away without a
> special check for O_TRUNC in the security_file_open hook.
> 
> The exact semantics might be slightly different to Capsicum though -
> in particular, the creat() call (= open with O_TRUNC|O_CREAT|O_WRONLY)
> will require the Landlock truncate right when it's overwriting an
> existing regular file, but it will not require the Landlock truncate
> right when it's creating a new file.

Is the creat() check really different from what is done by Capsicum?


> 
> I'm not fully sure how this is done in Capsicum. I assume that the
> Comparison with Capsicum is mostly for inspiration, but there is no
> goal of being fully compatible with that model?

I think Landlock has all the technical requirements to implement a 
Capsicum-like on Linux: unprivileged access control (which implies 
scoped access control, policies composition, only new restrictions, 
nesting, dedicated syscalls…). The main difference with the actual 
Landlock sandboxing would be that restrictions would apply to all 
processes doing actions on a specific kind of file descriptor (i.e. 
capability). Instead of checking the current thread's domain, Landlock 
could check the "file descriptor's domain". We're definitely not there 
yet but let's keep this in mind. ;)


> 
> The creat() behaviour is non-intuitive from userspace, I think:
> creat() is a pretty common way to create new files, and it might come
> as a surprise to people that this can require the truncate right,
> because:
> 
> - The function creat() doesn't have "truncate" in its name, and you
>    might be tempted to think that the LANDLOCK_ACCESS_FS_MAKE_REG is
>    sufficient for calling it.
> 
> - Users can work around the need for the truncate right by unlinking
>    the existing regular file with the same name and creating a new one.
>    So for the most common use case (where users do not care about the
>    file's inode identity or race conditions), it is surprising that
>    the truncate right is required.

These are useful information to put in the documentation. Explaining why 
it is required should help users. From my point of view, the logic 
behind is that replacing a file modifies its content (i.e. shrink it to 
zero), while unlinking a file doesn't change its content but makes it 
unreachable (removes it) from a directory (and it might not be deleted 
if linked elsewhere).


> 
> Summarizing this, I also think that the truncate right needs to be a
> separate flag, even if just for backwards compatibility reasons.
> 
> But at the same time, I suspect that in practice, the truncate right
> will probably have to usually go together with the file_write right,
> so that the very common creat() use case (and possibly others) does
> not yield surprising behaviour.

Agree. User space libraries might (and probably should) have a different 
interface than the raw syscalls. The Landlock syscalls are meant to 
provide a flexible interface for different use cases. We should keep in 
mind that the goal of libraries is to help developers. ;)

Re: [PATCH 0/2] landlock: truncate(2) support

[Günther Noack]

On Fri, Jul 29, 2022 at 01:58:17PM +0200, Mickaël Salaün wrote:
>
> On 10/07/2022 11:57, Günther Noack wrote:
> > Hello Mickaël!
> >
> > Thank you for the fast feedback! I'm looking into your comments from
> > this mail and the rest of the thread and am working on an updated
> > patch set.
> >
> > On Fri, Jul 08, 2022 at 01:16:29PM +0200, Mickaël Salaün wrote:
> > > Hi Günther, this looks good!
> > >
> > > Added linux-fsdevel@vger.kernel.org
> > >
> > > On 07/07/2022 22:06, Günther Noack wrote:
> > > > The goal of these patches is to work towards a more complete coverage
> > > > of file system operations that are restrictable with Landlock.
> > > >
> > > > The known set of currently unsupported file system operations in
> > > > Landlock is described at [1]. Out of the operations listed there,
> > > > truncate is the only one that modifies file contents, so these patches
> > > > should make it possible to prevent the direct modification of file
> > > > contents with Landlock.
> > > >
> > > > The patch introduces the truncate(2) restriction feature as an
> > > > additional bit in the access_mask_t bitmap, in line with the existing
> > > > supported operations.
> > > >
> > > > Apart from Landlock, the truncate(2) and ftruncate(2) family of system
> > > > calls can also be restricted using seccomp-bpf, but it is a
> > > > complicated mechanism (requires BPF, requires keeping up-to-date
> > > > syscall lists) and it also is not configurable by file hierarchy, as
> > > > Landlock is. The simplicity and flexibility of the Landlock approach
> > > > makes it worthwhile adding.
> > > >
> > > > I am aware that the documentation and samples/landlock/sandboxer.c
> > > > tool still need corresponding updates; I'm hoping to get some early
> > > > feedback this way.
> > > Yes, that's a good approach.
> > >
> > > Extending the sandboxer should be straightforward, you can just extend the
> > > scope of LL_FS_RW, taking into account the system Landlock ABI because there
> > > is no "contract" for this sample.
> >
> > Sounds good, I'll extend the sample tool like this for the updated patch set.
> >
> > (On the side, as you know from the discussion on the go-landlock
> > library, I have some suspicion that the "best effort"
> > backwards-compatibility approach in the sample tool is not the right
> > one for the "refer" right, but that might be better suited for a
> > separate patch. Maybe it'll be simpler to just not support a
> > best-effort downgrade in the sample tool.)
>
> Please share your though about the "refer" right.

The sample tool implements a "best effort" approach by removing the
access rights from all bitmasks passed to the kernel -- but this means
different things for the refer right than it does for other rights
like truncate:

* In the case of truncate, removing the truncate right from the
  handled rights means that truncate *will* be permitted after
  enforcement.

* In the case of "refer", removing the refer right from the handled
  rights means that the "refer" operations *will not* be permitted
  after enforcement.

Consequently, the approach of downgrading these needs to be different.

If the caller *asks* for the "refer" right to be permitted for a file
hierarchy, this cannot be done with Landlock ABI v1. Therefore, the
"best effort" downgrade will have to fall back to "doing nothing".

I've described this previously in this document:
https://docs.google.com/document/d/1SkFpl_Xxyl4E6G2uYIlzL0gY2PFo-Nl8ikblLvnpvlU/edit

Admittedly, this line of reasoning is more relevant to the proper
Landlock libraries than it is to the sample tool. However, the sample
tool is the place that people look at to understand the API... maybe
there should at least be a comment about it.

But as I said, this problem existed before the truncate patch already,
so it's probably best discussed separately; I'm happy to send a
separate patch if you agree with this line of reasoning.

>
>
> >
> > > You'll need to remove the warning about truncate(2) in the documentation,
> > > and maybe to move it to the "previous limitations" section, with the
> > > LANDLOCK_ACCESS_TRUNCATE doc pointing to it. I think it would be nice to
> > > extend the LANDLOCK_ACCESS_FS_WRITE documentation to point to
> > > LANDLOCK_ACCESS_FS_TRUNCATE because this distinction could be disturbing for
> > > users. Indeed, all inode-based LSMs (SELinux and Smack) deny such action if
> > > the inode is not writable (with the inode_permission check), which is not
> > > the case for path-based LSMs (AppArmor and Tomoyo).
> >
> > This makes a lot of sense, I'll work on the documentation to point this out.
> >
> > I suspect that for many common use cases, the
> > LANDLOCK_ACCESS_FS_TRUNCATE right will anyway only be used together
> > with LANDLOCK_ACCESS_FS_FILE_WRITE in practice. (See below for more
> > detail.)
>
> Agree
>
>
> >
> > > While we may question whether a dedicated access right should be added for
> > > the Landlock use case, two arguments are in favor of this approach:
> > > - For compatibility reasons, the kernel must follow the semantic of a
> > > specific Landlock ABI, otherwise it could break user space. We could still
> > > backport this patch and merge it with the ABI 1 and treat it as a bug, but
> > > the initial version of Landlock was meant to be an MVP, hence this lack of
> > > access right.
> > > - There is a specific access right for Capsicum (CAP_FTRUNCATE) that could
> > > makes more sense in the future.
> > >
> > > Following the Capsicum semantic, I think it would be a good idea to also
> > > check for the O_TRUNC open flag:
> > > https://www.freebsd.org/cgi/man.cgi?query=rights
> >
> > open() with O_TRUNC was indeed a case I had not thought about - thanks
> > for pointing it out.
> >
> > I started adding some tests for it, and found to my surprise that
> > open() *is* already checking security_path_truncate() when it is
> > truncating files. So there is a chance that we can get away without a
> > special check for O_TRUNC in the security_file_open hook.
> >
> > The exact semantics might be slightly different to Capsicum though -
> > in particular, the creat() call (= open with O_TRUNC|O_CREAT|O_WRONLY)
> > will require the Landlock truncate right when it's overwriting an
> > existing regular file, but it will not require the Landlock truncate
> > right when it's creating a new file.
>
> Is the creat() check really different from what is done by Capsicum?

TBH, I'm not sure, it might also do the same thing. I don't have a
FreeBSD machine at hand and am not familiar with Capsicum in detail.
Let me know if you think we should go to the effort of ensuring the
compatibility down to that level.

> > I'm not fully sure how this is done in Capsicum. I assume that the
> > Comparison with Capsicum is mostly for inspiration, but there is no
> > goal of being fully compatible with that model?
>
> I think Landlock has all the technical requirements to implement a
> Capsicum-like on Linux: unprivileged access control (which implies scoped
> access control, policies composition, only new restrictions, nesting,
> dedicated syscalls…). The main difference with the actual Landlock
> sandboxing would be that restrictions would apply to all processes doing
> actions on a specific kind of file descriptor (i.e. capability). Instead of
> checking the current thread's domain, Landlock could check the "file
> descriptor's domain". We're definitely not there yet but let's keep this in
> mind. ;)

Acknowledged.

>
>
> >
> > The creat() behaviour is non-intuitive from userspace, I think:
> > creat() is a pretty common way to create new files, and it might come
> > as a surprise to people that this can require the truncate right,
> > because:
> >
> > - The function creat() doesn't have "truncate" in its name, and you
> >    might be tempted to think that the LANDLOCK_ACCESS_FS_MAKE_REG is
> >    sufficient for calling it.
> >
> > - Users can work around the need for the truncate right by unlinking
> >    the existing regular file with the same name and creating a new one.
> >    So for the most common use case (where users do not care about the
> >    file's inode identity or race conditions), it is surprising that
> >    the truncate right is required.
>
> These are useful information to put in the documentation. Explaining why it
> is required should help users. From my point of view, the logic behind is
> that replacing a file modifies its content (i.e. shrink it to zero), while
> unlinking a file doesn't change its content but makes it unreachable
> (removes it) from a directory (and it might not be deleted if linked
> elsewhere).

Added it to the documentation with some rewording.

>
>
> >
> > Summarizing this, I also think that the truncate right needs to be a
> > separate flag, even if just for backwards compatibility reasons.
> >
> > But at the same time, I suspect that in practice, the truncate right
> > will probably have to usually go together with the file_write right,
> > so that the very common creat() use case (and possibly others) does
> > not yield surprising behaviour.
>
> Agree. User space libraries might (and probably should) have a different
> interface than the raw syscalls. The Landlock syscalls are meant to provide
> a flexible interface for different use cases. We should keep in mind that
> the goal of libraries is to help developers. ;)

--

Re: [PATCH 0/2] landlock: truncate(2) support

[Mickaël Salaün]

On 04/08/2022 18:10, Günther Noack wrote:
> On Fri, Jul 29, 2022 at 01:58:17PM +0200, Mickaël Salaün wrote:

[...]

>>>> While we may question whether a dedicated access right should be added for
>>>> the Landlock use case, two arguments are in favor of this approach:
>>>> - For compatibility reasons, the kernel must follow the semantic of a
>>>> specific Landlock ABI, otherwise it could break user space. We could still
>>>> backport this patch and merge it with the ABI 1 and treat it as a bug, but
>>>> the initial version of Landlock was meant to be an MVP, hence this lack of
>>>> access right.
>>>> - There is a specific access right for Capsicum (CAP_FTRUNCATE) that could
>>>> makes more sense in the future.
>>>>
>>>> Following the Capsicum semantic, I think it would be a good idea to also
>>>> check for the O_TRUNC open flag:
>>>> https://www.freebsd.org/cgi/man.cgi?query=rights
>>>
>>> open() with O_TRUNC was indeed a case I had not thought about - thanks
>>> for pointing it out.
>>>
>>> I started adding some tests for it, and found to my surprise that
>>> open() *is* already checking security_path_truncate() when it is
>>> truncating files. So there is a chance that we can get away without a
>>> special check for O_TRUNC in the security_file_open hook.
>>>
>>> The exact semantics might be slightly different to Capsicum though -
>>> in particular, the creat() call (= open with O_TRUNC|O_CREAT|O_WRONLY)
>>> will require the Landlock truncate right when it's overwriting an
>>> existing regular file, but it will not require the Landlock truncate
>>> right when it's creating a new file.
>>
>> Is the creat() check really different from what is done by Capsicum?
> 
> TBH, I'm not sure, it might also do the same thing. I don't have a
> FreeBSD machine at hand and am not familiar with Capsicum in detail.
> Let me know if you think we should go to the effort of ensuring the
> compatibility down to that level.

I'll take a look at the code, but it makes sense to implement it like 
you did.

Re: [PATCH 1/2] landlock: Support truncate(2).

[Mickaël Salaün]

No final dot for a subject please.

On 07/07/2022 22:06, Günther Noack wrote:
> Add support for restricting the use of the truncate(2) and
> ftruncate(2) family of syscalls with Landlock.
> 
> This change also updates the Landlock ABI version and updates the
> existing Landlock tests to match the new ABI version.
> 
> Technically, unprivileged processes can already restrict the use of
> truncate(2) with seccomp-bpf.
> 
> Using Landlock instead of seccomp-bpf has the folowwing advantages:

typo: following

> 
> - it doesn't require the use of BPF (conceptually simpler)
> 
> - callers don't need to keep track of lists of syscall numbers for
>    different architectures and kernel versions
> 
> - the restriction policy can be configured per file hierarchy.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   include/uapi/linux/landlock.h                | 2 ++
>   security/landlock/fs.c                       | 9 ++++++++-
>   security/landlock/limits.h                   | 2 +-
>   security/landlock/syscalls.c                 | 2 +-
>   tools/testing/selftests/landlock/base_test.c | 2 +-
>   tools/testing/selftests/landlock/fs_test.c   | 7 ++++---
>   6 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..2351050d4773 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -134,6 +134,7 @@ struct landlock_path_beneath_attr {
>    *   directory) parent.  Otherwise, such actions are denied with errno set to
>    *   EACCES.  The EACCES errno prevails over EXDEV to let user space
>    *   efficiently deal with an unrecoverable error.
> + * - %LANDLOCK_ACCESS_FS_TRUNCATE%: Truncate a file.

We need to specify the ABI version starting to support this right.

>    *
>    * .. warning::

You need to remove truncate(2) from this warning block.

>    *
> @@ -160,6 +161,7 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
> +#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>   /* clang-format on */
>   
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ec5a6247cd3e..c57f581a9cd5 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   /* clang-format on */
>   
>   /*
> @@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
>   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>   }
>   
> +static int hook_path_truncate(const struct path *const path)
> +{
> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> +}
> +
>   /* File hooks */
>   
>   static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
>   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
> +	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
>   
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   };
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index b54184ab9439..82288f0e9e5e 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>   #define LANDLOCK_MAX_NUM_LAYERS		16
>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>   
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>   
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 735a0865ea11..f4d6fc7ed17f 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>   	.write = fop_dummy_write,
>   };
>   
> -#define LANDLOCK_ABI_VERSION 2
> +#define LANDLOCK_ABI_VERSION 3
>   
>   /**
>    * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index da9290817866..72cdae277b02 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>   	const struct landlock_ruleset_attr ruleset_attr = {
>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>   	};
> -	ASSERT_EQ(2, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
>   					     LANDLOCK_CREATE_RULESET_VERSION));
>   
>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..cb77eaa01c91 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
>   
>   #define ACCESS_ALL ( \
>   	ACCESS_FILE | \
> @@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
>   	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
>   	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
>   	LANDLOCK_ACCESS_FS_MAKE_SYM | \
> -	ACCESS_LAST)
> +	LANDLOCK_ACCESS_FS_REFER)

I created ACCESS_LAST to store the last access right while avoiding to 
copy it in ACCESS_FILE or ACCESS_ALL, and then avoid forgetting about 
new access right, but I now think it is not worth it and I prefer your 
approach which will be easier to maintain.

>   
>   /* clang-format on */
>   

Re: [PATCH 1/2] landlock: Support truncate(2).

[Günther Noack]

On Fri, Jul 08, 2022 at 01:17:32PM +0200, Mickaël Salaün wrote:
> No final dot for a subject please.

Done. Will be fixed in the next version.

>
> On 07/07/2022 22:06, Günther Noack wrote:
> > Add support for restricting the use of the truncate(2) and
> > ftruncate(2) family of syscalls with Landlock.
> >
> > This change also updates the Landlock ABI version and updates the
> > existing Landlock tests to match the new ABI version.
> >
> > Technically, unprivileged processes can already restrict the use of
> > truncate(2) with seccomp-bpf.
> >
> > Using Landlock instead of seccomp-bpf has the folowwing advantages:
>
> typo: following

Done. Will be fixed in the next version.

>
> >
> > - it doesn't require the use of BPF (conceptually simpler)
> >
> > - callers don't need to keep track of lists of syscall numbers for
> >    different architectures and kernel versions
> >
> > - the restriction policy can be configured per file hierarchy.
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   include/uapi/linux/landlock.h                | 2 ++
> >   security/landlock/fs.c                       | 9 ++++++++-
> >   security/landlock/limits.h                   | 2 +-
> >   security/landlock/syscalls.c                 | 2 +-
> >   tools/testing/selftests/landlock/base_test.c | 2 +-
> >   tools/testing/selftests/landlock/fs_test.c   | 7 ++++---
> >   6 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..2351050d4773 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -134,6 +134,7 @@ struct landlock_path_beneath_attr {
> >    *   directory) parent.  Otherwise, such actions are denied with errno set to
> >    *   EACCES.  The EACCES errno prevails over EXDEV to let user space
> >    *   efficiently deal with an unrecoverable error.
> > + * - %LANDLOCK_ACCESS_FS_TRUNCATE%: Truncate a file.
>
> We need to specify the ABI version starting to support this right.

Done. Will be fixed in the next version.

>
> >    *
> >    * .. warning::
>
> You need to remove truncate(2) from this warning block.

Done. Will be fixed in the next version.

>
> >    *
> > @@ -160,6 +161,7 @@ struct landlock_path_beneath_attr {
> >   #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
> >   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
> >   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
> > +#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
> >   /* clang-format on */
> >   #endif /* _UAPI_LINUX_LANDLOCK_H */
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index ec5a6247cd3e..c57f581a9cd5 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
> >   #define ACCESS_FILE ( \
> >   	LANDLOCK_ACCESS_FS_EXECUTE | \
> >   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> > -	LANDLOCK_ACCESS_FS_READ_FILE)
> > +	LANDLOCK_ACCESS_FS_READ_FILE | \
> > +	LANDLOCK_ACCESS_FS_TRUNCATE)
> >   /* clang-format on */
> >   /*
> > @@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
> >   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> >   }
> > +static int hook_path_truncate(const struct path *const path)
> > +{
> > +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> > +}
> > +
> >   /* File hooks */
> >   static inline access_mask_t get_file_access(const struct file *const file)
> > @@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> >   	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
> >   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
> >   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
> > +	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
> >   	LSM_HOOK_INIT(file_open, hook_file_open),
> >   };
> > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > index b54184ab9439..82288f0e9e5e 100644
> > --- a/security/landlock/limits.h
> > +++ b/security/landlock/limits.h
> > @@ -18,7 +18,7 @@
> >   #define LANDLOCK_MAX_NUM_LAYERS		16
> >   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
> > -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
> > +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> >   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
> >   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
> > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > index 735a0865ea11..f4d6fc7ed17f 100644
> > --- a/security/landlock/syscalls.c
> > +++ b/security/landlock/syscalls.c
> > @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
> >   	.write = fop_dummy_write,
> >   };
> > -#define LANDLOCK_ABI_VERSION 2
> > +#define LANDLOCK_ABI_VERSION 3
> >   /**
> >    * sys_landlock_create_ruleset - Create a new ruleset
> > diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> > index da9290817866..72cdae277b02 100644
> > --- a/tools/testing/selftests/landlock/base_test.c
> > +++ b/tools/testing/selftests/landlock/base_test.c
> > @@ -75,7 +75,7 @@ TEST(abi_version)
> >   	const struct landlock_ruleset_attr ruleset_attr = {
> >   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
> >   	};
> > -	ASSERT_EQ(2, landlock_create_ruleset(NULL, 0,
> > +	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
> >   					     LANDLOCK_CREATE_RULESET_VERSION));
> >   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 21a2ce8fa739..cb77eaa01c91 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
> >   #define ACCESS_FILE ( \
> >   	LANDLOCK_ACCESS_FS_EXECUTE | \
> >   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> > -	LANDLOCK_ACCESS_FS_READ_FILE)
> > +	LANDLOCK_ACCESS_FS_READ_FILE | \
> > +	LANDLOCK_ACCESS_FS_TRUNCATE)
> > -#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
> > +#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
> >   #define ACCESS_ALL ( \
> >   	ACCESS_FILE | \
> > @@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
> >   	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
> >   	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
> >   	LANDLOCK_ACCESS_FS_MAKE_SYM | \
> > -	ACCESS_LAST)
> > +	LANDLOCK_ACCESS_FS_REFER)
>
> I created ACCESS_LAST to store the last access right while avoiding to copy
> it in ACCESS_FILE or ACCESS_ALL, and then avoid forgetting about new access
> right, but I now think it is not worth it and I prefer your approach which
> will be easier to maintain.

OK, thanks.

>
> >   /* clang-format on */

--

Re: [PATCH 2/2] landlock: Selftests for truncate(2) support.

[Mickaël Salaün]

Please use "selftests/landlock:" as subject prefix and without a final dot.


On 07/07/2022 22:06, Günther Noack wrote:
> These tests exercise the following scenarios:
> 
> * File with Read, Write, Truncate rights.

Should we use a capital for access right names or does it come from Go? ;)


> * File with Read, Write rights.
> * File with Truncate rights.
> * File with no rights.
> * Directory with Truncate rights.
> 
> For each of the scenarios, both truncate() and the open() +
> ftruncate() syscalls get exercised and their results checked.
> 
> In particular, the test demonstrates that opening a file for writing
> is not enough to call truncate().

Looks good! According to my previous comment, O_TRUNC should be tested 
if it is checked by the kernel.


> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 80 ++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..c3e48fd12b2b 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2237,6 +2237,86 @@ TEST_F_FORK(layout1, reparent_rename)
>   	ASSERT_EQ(EXDEV, errno);
>   }
>   
> +TEST_F_FORK(layout1, truncate)

Please move this test after the proc_pipe one.


> +{
> +	const struct rule rules[] = {

You can add a first layer of rules to check truncate and ftruncate with 
a ruleset not handling LANDLOCK_ACCESS_FS_TRUNCATE.


> +		{
> +			.path = file1_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file2_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file1_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},

Move this entry before file2_s1d2 to keep the paths sorted and make this 
easier to read. You can change the access rights per path to also keep 
their ordering though.


> +		{
> +			.path = dir_s2d3,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		// Implicitly: No access rights for file2_s1d1.

Comment to move after the use of file1_s1d1.

> +		{},
> +	};
> +	const int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, rules);

Don't use ACCESS_ALL because it will change over time and we want tests 
to be deterministic. You can use rules[0].access instead.


> +	int reg_fd;
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Read, write and truncate permissions => truncate and ftruncate work. */

It would be nice to have consistent comments such as: "Checks read, 
write and truncate access rights: truncate and ftruncate work."


> +	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(0, ftruncate(reg_fd, 10));

You should not use EXPECT but ASSERT here. I use EXPECT when an error 
could block a test or when it could stop a cleanup (i.e. teardown).


> +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	EXPECT_EQ(0, truncate(file1_s1d1, 10));
> +	EXPECT_EQ(0, truncate64(file1_s1d1, 20));
> +
> +	/* Just read and write permissions => no truncate variant works. */
> +	reg_fd = open(file2_s1d2, O_RDWR | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(-1, ftruncate(reg_fd, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, ftruncate64(reg_fd, 20));
> +	EXPECT_EQ(EACCES, errno);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	EXPECT_EQ(-1, truncate(file2_s1d2, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file2_s1d2, 20));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Just truncate permissions => truncate(64) works, but can't open file. */
> +	ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file1_s1d2, 10));
> +	EXPECT_EQ(0, truncate64(file1_s1d2, 20));
> +
> +	/* Just truncate permission on directory => truncate(64) works, but can't open file. */
> +	ASSERT_EQ(-1, open(file1_s2d3, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file1_s2d3, 10));
> +	EXPECT_EQ(0, truncate64(file1_s2d3, 20));
> +
> +	/* No permissions => Neither truncate nor ftruncate work. */
> +	ASSERT_EQ(-1, open(file2_s1d1, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, truncate(file2_s1d1, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file2_s1d1, 20));
> +	EXPECT_EQ(EACCES, errno);

These tests are good!

> +}
> +
>   static void
>   reparent_exdev_layers_enforce1(struct __test_metadata *const _metadata)
>   {

Re: [PATCH 2/2] landlock: Selftests for truncate(2) support.

[Günther Noack]

On Fri, Jul 08, 2022 at 01:17:46PM +0200, Mickaël Salaün wrote:
> Please use "selftests/landlock:" as subject prefix and without a final dot.
>
>
> On 07/07/2022 22:06, Günther Noack wrote:
> > These tests exercise the following scenarios:
> >
> > * File with Read, Write, Truncate rights.
>
> Should we use a capital for access right names or does it come from Go? ;)

Done. Will be included in the next version.

>
>
> > * File with Read, Write rights.
> > * File with Truncate rights.
> > * File with no rights.
> > * Directory with Truncate rights.
> >
> > For each of the scenarios, both truncate() and the open() +
> > ftruncate() syscalls get exercised and their results checked.
> >
> > In particular, the test demonstrates that opening a file for writing
> > is not enough to call truncate().
>
> Looks good! According to my previous comment, O_TRUNC should be tested if it
> is checked by the kernel.

Done. Will be included in the next version.

>
>
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   tools/testing/selftests/landlock/fs_test.c | 80 ++++++++++++++++++++++
> >   1 file changed, 80 insertions(+)
> >
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index cb77eaa01c91..c3e48fd12b2b 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -2237,6 +2237,86 @@ TEST_F_FORK(layout1, reparent_rename)
> >   	ASSERT_EQ(EXDEV, errno);
> >   }
> > +TEST_F_FORK(layout1, truncate)
>
> Please move this test after the proc_pipe one.

Done. Will be included in the next version.

>
>
> > +{
> > +	const struct rule rules[] = {
>
> You can add a first layer of rules to check truncate and ftruncate with a
> ruleset not handling LANDLOCK_ACCESS_FS_TRUNCATE.

Done. I'll add a separate test for that which will exercise the
various truncation APIs in a scenario where the ruleset does not
handle LANDLOCK_ACCESS_FS_TRUNCATE, so that it's not restricted.

Will be included in the next version.

>
>
> > +		{
> > +			.path = file1_s1d1,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> > +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		{
> > +			.path = file2_s1d2,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		{
> > +			.path = file1_s1d2,
> > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
>
> Move this entry before file2_s1d2 to keep the paths sorted and make this
> easier to read. You can change the access rights per path to also keep their
> ordering though.

I've admittedly found it difficult to remember which of these files
and subdirectories exist and how they are named and mixed up the names
at least twice when developing these tests. To make it easier, I've now
renamed these by including this at the top of the test:

char *file_rwt = file1_s1d1;
char *file_rw = file2_s1s1;
// etc

With the test using names like file_rwt, I find that easier to work
with and found myself jumping less between the "rules" on top and the
place where the assertions are written out.

This is admittedly a bit out of line with the other tests, but maybe
it's worth doing? Let me know what you think.

>
>
> > +		{
> > +			.path = dir_s2d3,
> > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		// Implicitly: No access rights for file2_s1d1.
>
> Comment to move after the use of file1_s1d1.

I'm understanding this as "keep the files in order according to the
layout". I've reshuffled things a bit by renaming them, but this is
also in the right order now.

>
> > +		{},
> > +	};
> > +	const int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, rules);
>
> Don't use ACCESS_ALL because it will change over time and we want tests to
> be deterministic. You can use rules[0].access instead.
>
>
> > +	int reg_fd;
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Read, write and truncate permissions => truncate and ftruncate work. */
>
> It would be nice to have consistent comments such as: "Checks read, write
> and truncate access rights: truncate and ftruncate work."

Done. Will be included in next version.

>
>
> > +	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
>
> You should not use EXPECT but ASSERT here. I use EXPECT when an error could
> block a test or when it could stop a cleanup (i.e. teardown).

ASSERT is the variant that stops the test immediately, whereas EXPECT
notes down the test failure and continues execution.

So in that spirit, I tried to use:

 * ASSERT for successful open() calls where the FD is still needed later
 * ASSERT for close() (for symmetry with open())
 * EXPECT for expected-failing open() calls where the FD is not used later
 * EXPECT for everything else

I had another pass over the tests and have started to use EXPECT for a
few expected-failing open() calls.

The selftest framework seems inspired by the Googletest framework
(https://google.github.io/googletest/primer.html#assertions) where
this is described as: "Usually EXPECT_* are preferred, as they allow
more than one failure to be reported in a test. However, you should
use ASSERT_* if it doesn’t make sense to continue when the assertion
in question fails."

I imagined that the same advice would apply to the kernel selftests?
Please let me know if I'm overlooking subtle differences here.

>
>
> > +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	EXPECT_EQ(0, truncate(file1_s1d1, 10));
> > +	EXPECT_EQ(0, truncate64(file1_s1d1, 20));
> > +
> > +	/* Just read and write permissions => no truncate variant works. */
> > +	reg_fd = open(file2_s1d2, O_RDWR | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	EXPECT_EQ(-1, ftruncate(reg_fd, 10));
> > +	EXPECT_EQ(EACCES, errno);
> > +	EXPECT_EQ(-1, ftruncate64(reg_fd, 20));
> > +	EXPECT_EQ(EACCES, errno);
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	EXPECT_EQ(-1, truncate(file2_s1d2, 10));
> > +	EXPECT_EQ(EACCES, errno);
> > +	EXPECT_EQ(-1, truncate64(file2_s1d2, 20));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	/* Just truncate permissions => truncate(64) works, but can't open file. */
> > +	ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
> > +	ASSERT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(0, truncate(file1_s1d2, 10));
> > +	EXPECT_EQ(0, truncate64(file1_s1d2, 20));
> > +
> > +	/* Just truncate permission on directory => truncate(64) works, but can't open file. */
> > +	ASSERT_EQ(-1, open(file1_s2d3, O_RDWR | O_CLOEXEC));
> > +	ASSERT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(0, truncate(file1_s2d3, 10));
> > +	EXPECT_EQ(0, truncate64(file1_s2d3, 20));
> > +
> > +	/* No permissions => Neither truncate nor ftruncate work. */
> > +	ASSERT_EQ(-1, open(file2_s1d1, O_RDWR | O_CLOEXEC));
> > +	ASSERT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, truncate(file2_s1d1, 10));
> > +	EXPECT_EQ(EACCES, errno);
> > +	EXPECT_EQ(-1, truncate64(file2_s1d1, 20));
> > +	EXPECT_EQ(EACCES, errno);
>
> These tests are good!

Thanks :)

>
> > +}
> > +
> >   static void
> >   reparent_exdev_layers_enforce1(struct __test_metadata *const _metadata)
> >   {

--

Re: [PATCH 2/2] landlock: Selftests for truncate(2) support.

[Mickaël Salaün]

On 11/07/2022 18:27, Günther Noack wrote:
> On Fri, Jul 08, 2022 at 01:17:46PM +0200, Mickaël Salaün wrote:

[...]

>>
>>> +		{
>>> +			.path = file1_s1d1,
>>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
>>> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		{
>>> +			.path = file2_s1d2,
>>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>>> +		},
>>> +		{
>>> +			.path = file1_s1d2,
>>> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>
>> Move this entry before file2_s1d2 to keep the paths sorted and make this
>> easier to read. You can change the access rights per path to also keep their
>> ordering though.
> 
> I've admittedly found it difficult to remember which of these files
> and subdirectories exist and how they are named and mixed up the names
> at least twice when developing these tests. To make it easier, I've now
> renamed these by including this at the top of the test:
> 
> char *file_rwt = file1_s1d1;
> char *file_rw = file2_s1s1;
> // etc
> 
> With the test using names like file_rwt, I find that easier to work
> with and found myself jumping less between the "rules" on top and the
> place where the assertions are written out.
> 
> This is admittedly a bit out of line with the other tests, but maybe
> it's worth doing? Let me know what you think.

It indeed makes things clearer.


> 
>>
>>
>>> +		{
>>> +			.path = dir_s2d3,
>>> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		// Implicitly: No access rights for file2_s1d1.
>>
>> Comment to move after the use of file1_s1d1.
> 
> I'm understanding this as "keep the files in order according to the
> layout". I've reshuffled things a bit by renaming them, but this is
> also in the right order now.

Right.

[...]

>>> +	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
>>> +	ASSERT_LE(0, reg_fd);
>>> +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
>>
>> You should not use EXPECT but ASSERT here. I use EXPECT when an error could
>> block a test or when it could stop a cleanup (i.e. teardown).
> 
> ASSERT is the variant that stops the test immediately, whereas EXPECT
> notes down the test failure and continues execution.
> 
> So in that spirit, I tried to use:
> 
>   * ASSERT for successful open() calls where the FD is still needed later
>   * ASSERT for close() (for symmetry with open())
>   * EXPECT for expected-failing open() calls where the FD is not used later
>   * EXPECT for everything else

I understand your logic, but this gymnastic adds complexity to writing 
tests (which might be difficult to explain) for not much gain. Indeed, 
all these tests should pass, except if we add a SKIP (cf. 
https://lore.kernel.org/all/20220628222941.2642917-1-jeffxu@google.com/).

In the case of an open FD, it will not be an issue to not close it if a 
test failed, which is not the same with FIXTURE_TEARDOWN where we want 
the workspace to be clean after tests, whether they succeeded or failed.


> 
> I had another pass over the tests and have started to use EXPECT for a
> few expected-failing open() calls.
> 
> The selftest framework seems inspired by the Googletest framework
> (https://google.github.io/googletest/primer.html#assertions) where
> this is described as: "Usually EXPECT_* are preferred, as they allow
> more than one failure to be reported in a test. However, you should
> use ASSERT_* if it doesn’t make sense to continue when the assertion
> in question fails."

I think this is good in theory, but in practice, at least for the 
Landlock selftests, everything should pass. Any test failure is a 
blocker because it breaks the contract with users.

I find it very difficult to write tests that would check as much as 
possible, even if some of these tests failed, without unexpected 
behaviors (e.g. blocking the whole tests, writing to unexpected 
locations…) because it changes the previous state from a known state to 
a set of potential states (e.g. when creating or removing files). Doing 
it generically increases complexity for tests which may already be 
difficult to understand. When investigating a test failure, we can still 
replace some ASSERT with EXPECT though.


> 
> I imagined that the same advice would apply to the kernel selftests?
> Please let me know if I'm overlooking subtle differences here.

I made kselftest_harness.h generally available (outside of seccomp) but 
I guess each subsystem maintainer might handle that differently.

See 
https://lore.kernel.org/all/1b043379-b6eb-d272-c9b9-25c6960e1ef1@digikod.net/ 
for similar concerns.

Re: [PATCH 2/2] landlock: Selftests for truncate(2) support.

[Günther Noack]

On Fri, Jul 29, 2022 at 01:30:24PM +0200, Mickaël Salaün wrote:
> On 11/07/2022 18:27, Günther Noack wrote:
> > On Fri, Jul 08, 2022 at 01:17:46PM +0200, Mickaël Salaün wrote:
>
> [...]
>
> > >
> > > > +		{
> > > > +			.path = file1_s1d1,
> > > > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > > > +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> > > > +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> > > > +		},
> > > > +		{
> > > > +			.path = file2_s1d2,
> > > > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > > > +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > > +		},
> > > > +		{
> > > > +			.path = file1_s1d2,
> > > > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > > > +		},
> > >
> > > Move this entry before file2_s1d2 to keep the paths sorted and make this
> > > easier to read. You can change the access rights per path to also keep their
> > > ordering though.
> >
> > I've admittedly found it difficult to remember which of these files
> > and subdirectories exist and how they are named and mixed up the names
> > at least twice when developing these tests. To make it easier, I've now
> > renamed these by including this at the top of the test:
> >
> > char *file_rwt = file1_s1d1;
> > char *file_rw = file2_s1s1;
> > // etc
> >
> > With the test using names like file_rwt, I find that easier to work
> > with and found myself jumping less between the "rules" on top and the
> > place where the assertions are written out.
> >
> > This is admittedly a bit out of line with the other tests, but maybe
> > it's worth doing? Let me know what you think.
>
> It indeed makes things clearer.
>
>
> >
> > >
> > >
> > > > +		{
> > > > +			.path = dir_s2d3,
> > > > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > > > +		},
> > > > +		// Implicitly: No access rights for file2_s1d1.
> > >
> > > Comment to move after the use of file1_s1d1.
> >
> > I'm understanding this as "keep the files in order according to the
> > layout". I've reshuffled things a bit by renaming them, but this is
> > also in the right order now.
>
> Right.
>
> [...]
>
> > > > +	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
> > > > +	ASSERT_LE(0, reg_fd);
> > > > +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
> > >
> > > You should not use EXPECT but ASSERT here. I use EXPECT when an error could
> > > block a test or when it could stop a cleanup (i.e. teardown).
> >
> > ASSERT is the variant that stops the test immediately, whereas EXPECT
> > notes down the test failure and continues execution.
> >
> > So in that spirit, I tried to use:
> >
> >   * ASSERT for successful open() calls where the FD is still needed later
> >   * ASSERT for close() (for symmetry with open())
> >   * EXPECT for expected-failing open() calls where the FD is not used later
> >   * EXPECT for everything else
>
> I understand your logic, but this gymnastic adds complexity to writing tests
> (which might be difficult to explain) for not much gain. Indeed, all these
> tests should pass, except if we add a SKIP (cf.
> https://lore.kernel.org/all/20220628222941.2642917-1-jeffxu@google.com/).
>
> In the case of an open FD, it will not be an issue to not close it if a test
> failed, which is not the same with FIXTURE_TEARDOWN where we want the
> workspace to be clean after tests, whether they succeeded or failed.
>
>
> >
> > I had another pass over the tests and have started to use EXPECT for a
> > few expected-failing open() calls.
> >
> > The selftest framework seems inspired by the Googletest framework
> > (https://google.github.io/googletest/primer.html#assertions) where
> > this is described as: "Usually EXPECT_* are preferred, as they allow
> > more than one failure to be reported in a test. However, you should
> > use ASSERT_* if it doesn’t make sense to continue when the assertion
> > in question fails."
>
> I think this is good in theory, but in practice, at least for the Landlock
> selftests, everything should pass. Any test failure is a blocker because it
> breaks the contract with users.
>
> I find it very difficult to write tests that would check as much as
> possible, even if some of these tests failed, without unexpected behaviors
> (e.g. blocking the whole tests, writing to unexpected locations…) because it
> changes the previous state from a known state to a set of potential states
> (e.g. when creating or removing files). Doing it generically increases
> complexity for tests which may already be difficult to understand. When
> investigating a test failure, we can still replace some ASSERT with EXPECT
> though.

After some other refactorings you suggested (which I'll post soon),
the bulk of the test code now just consists of long stretches of

  EXPECT_EQ(0, test_open(...));
  EXPECT_EQ(0, test_truncate(...));
  EXPECT_EQ(EACCES, test_ftruncate(...));

So these are actually reasonably independent and don't interact much,
which means that printing multiple independent failures is not too
hard. There are a bunch of ASSERT usages left, but they are hidden in
the test_foo() helpers.

I suspect you would be ok with it now, and I'll try to send the next
patch version still with EXPECT. If you still feel strongly about it,
please let me know.

(The classic JUnit/XUnit test frameworks only have an equivalent to
ASSERT and crash early during tests. On the other hand, in these
frameworks, there is also more emphasis on writing a larger number of
narrower test cases instead of the long chains of assertions and
expectations that we use here.)

>
>
> >
> > I imagined that the same advice would apply to the kernel selftests?
> > Please let me know if I'm overlooking subtle differences here.
>
> I made kselftest_harness.h generally available (outside of seccomp) but I
> guess each subsystem maintainer might handle that differently.
>
> See
> https://lore.kernel.org/all/1b043379-b6eb-d272-c9b9-25c6960e1ef1@digikod.net/
> for similar concerns.

--