Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Sat, 02 Dec 2023 10:03:11 +0000
To:     Alice Ryhl <aliceryhl@google.com>
From:   Benno Lossin <benno.lossin@proton.me>
Cc:     a.hindborg@samsung.com, alex.gaynor@gmail.com, arve@android.com,
        bjorn3_gh@protonmail.com, boqun.feng@gmail.com, brauner@kernel.org,
        cmllamas@google.com, dan.j.williams@intel.com, dxu@dxuuu.xyz,
        gary@garyguo.net, gregkh@linuxfoundation.org,
        joel@joelfernandes.org, keescook@chromium.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        maco@android.com, ojeda@kernel.org, peterz@infradead.org,
        rust-for-linux@vger.kernel.org, surenb@google.com,
        tglx@linutronix.de, tkjos@android.com, viro@zeniv.linux.org.uk,
        wedsonaf@gmail.com, willy@infradead.org
Subject: Re: [PATCH 3/7] rust: security: add abstraction for secctx
Message-ID: <TdIzHaMBCR_0nM5Vvj7NWAG4feqbl2J7FcUgdCvXgHejuysujgtwXze0TEHNBpOWw26N4zgJzMvbfoFICQgPcmWfK4PyWl08MYIpxWuvPxE=@proton.me>
In-Reply-To: <20231201104831.2195715-1-aliceryhl@google.com>
References: <qwxqEq_l1jj3cAKSEh7gBZCUyBGCDmThdz6JJIQiFVl94ASI4yyNB6956XLrsQXnE4ulo48QRMaKPjgt7JZoolisVEiGOUP7IyRdecdhXqw=@proton.me> <20231201104831.2195715-1-aliceryhl@google.com>
Feedback-ID: 71780778:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 12/1/23 11:48, Alice Ryhl wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
>> On 11/29/23 14:11, Alice Ryhl wrote:
>>> +    /// Returns the bytes for this security context.
>>> +    pub fn as_bytes(&self) -> &[u8] {
>>> +        let mut ptr =3D self.secdata;
>>> +        if ptr.is_null() {
>>> +            // Many C APIs will use null pointers for strings of lengt=
h zero, but
>>
>> I would just write that the secctx API uses null pointers to denote a
>> string of length zero.
>=20
> I don't actually know whether it can ever be null, I just wanted to stay
> on the safe side.

I see, can someone from the C side confirm/refute this?

I found the comment a bit weird, since it is phrased in a general way.
If it turns out that the pointer can never be null, maybe use `NonNull`
instead (I would then also move the length check into the constructor)?
You can probably also do this if the pointer is allowed to be null,
assuming that you then do not need to call `security_release_secctx`.

>>> +            // `slice::from_raw_parts` doesn't allow the pointer to be=
 null even if the length is
>>> +            // zero. Replace the pointer with a dangling but non-null =
pointer in this case.
>>> +            debug_assert_eq!(self.seclen, 0);
>>
>> I am feeling a bit uncomfortable with this, why can't we just return
>> an empty slice in this case?
>=20
> I can do that, but to be clear, what I'm doing here is also definitely
> okay.

Yes it is okay, but I see this similar to avoiding `unsafe` code when it
can be done safely. In this example we are not strictly avoiding any
`unsafe` code, but we are avoiding a codepath with `unsafe` code. You
should of course still keep the `debug_assert_eq`.

--=20
Cheers,
Benno