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] [day] [month] [year] [list]

Date: Tue, 11 Jun 2024 14:35:41 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.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

6/10/2024 11:03 AM, Günther Noack wrote:
> 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

Thank you, Günther! It really looks more clear.

This patch should be applied to Landlock separately, right?