netdev - Re: [RFC PATCH v2 00/12] Socket type control for Landlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZmazTKVNlsH3crwP@google.com>
Date: Mon, 10 Jun 2024 10:03:24 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: "Günther Noack" <gnoack3000@...il.com>, mic@...ikod.net, willemdebruijn.kernel@...il.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	netfilter-devel@...r.kernel.org, yusongping@...wei.com, 
	artem.kuzin@...wei.com, konstantin.meskhidze@...wei.com, 
	Tahera Fahimi <fahimitahera@...il.com>
Subject: Re: [RFC PATCH v2 00/12] Socket type control for Landlock

On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
> 6/4/2024 11:22 PM, Günther Noack wrote:
> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
> really strange way (see landlock/limits.h):
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
> 
> With this definition, socket access mask overlaps the fs access
> mask in ruleset->access_masks[layer_level]. That's why
> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
> 
> So, the macro must be defined in this way:
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>                                          LANDLOCK_NUM_ACCESS_FS)
> 
> With this fix, open() doesn't fail in your example.
> 
> I'm really sorry that I somehow made such a stupid typo. I will try my
> best to make sure this doesn't happen again.

I found that we had the exact same bug with a wrongly defined "SHIFT" value in
[1].

Maybe we should define access_masks_t as a bit-field rather than doing the
bit-shifts by hand.  Then the compiler would keep track of the bit-offsets
automatically.

Bit-fields have a bad reputation, but in my understanding, this is largely
because they make it hard to control the exact bit-by-bit layout.  In our case,
we do not need such an exact control though, and it would be fine.

To quote Linus Torvalds on [2],

  Bitfields are fine if you don't actually care about the underlying format,
  and want gcc to just randomly assign bits, and want things to be
  convenient in that situation.

Let me send you a proposal patch which replaces access_masks_t with a bit-field
and removes the need for the "SHIFT" definition, which we already got wrong in
two patch sets now.  It has the additional benefit of making the code a bit
shorter and also removing a few static_assert()s which are now guaranteed by the
compiler.

—Günther

[1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/
[2] https://yarchive.net/comp/linux/bitfields.html