Re: Usage of mutex_t inside libc

[Ville Juven]

Hi,

I agree totally that the contents of the stack allocated wait object 
(whatever) do not need validation, and the user of course cannot access 
/ destroy the contents.

What I did not understand before inspecting the Linux kernel code, is 
how the wait list integrity is ensured when the stack allocated wait 
object (whatever) is destroyed. I did not understand where the list head 
resides. But after inspecting the Linux source more carefully I realized 
the global hash bucket contains the list head, so indeed it is safe to 
use a stack allocated list element, as long as the list is globally 
locked when inserting / removing.

I think I will not implement the hash bucket, I don't think O(n) 
complexity (insert) is acceptable for us, correct me if I'm wrong... ? 
Global locking also sounds kind of unnecessary. I'll do something else 
instead for the list heads. But this solution is workable I think.

Br,

Ville

On 27.4.2023 12.28, Xiang Xiao wrote:
Yes, this is what I said before. You don't need to validate the stack
allocated list since it comes from the kernel stack which can't be modified
or even read by the userspace code.

On Thu, Apr 27, 2023 at 3:43 PM Ville Juven <ville.ju...@gmail.com> wrote:

Hi,

I think Xiang refers to how Linux does it. It simply creates a new
"waitobj" variable into the kernel stack by declaring it inside the
sem_wait()-equivalent function. The wait object is created into the
kernel stack, and a hash is calculated for the user semaphore address.
The hash is used to identify the semaphore and the wait object. The hash
buckets exist in a massive global hash bucket that is allocated for the
kernel during init.


A very simplified example could be something like this:

struct waitobj_s

{

    dq_waitlist waitlist;

    struct semholder_s *holders;

}

int nxsem_wait(FAR sem_t *sem)
{

    if (sem->semcount > 0)

    {

      // No need to block, no need for a waitobj

    }

    else

    {

      // Need to wait / block, create a waitobj and go to sleep

      struct waitobj_s waitobj; // The "frame" I was talking about is
created here

      sem->waitobj = &waitobj; // Just an example, this is a BAD IDEA!
(At this point Linux would insert the object into the hash bucket)

      ...

    } // And the "frame" gets destroyed here

    return ...

}

The semantics of waitobj are wrong in my example, but the basic idea is
valid still. I don't know / understand (yet) how the validity of the
stack allocated lists is guaranteed.


Br,

Ville


On 27.4.2023 3.25, Gregory Nutt wrote:
I didn't do a careful analysis, but glancing at the ARMv7-A code, it
looks to me that none of the things needed to create stack frames are
available for the kernel stack.  It looks like the system call entry
logic just sets the SP to the top of the kernel stack and does very
little more.  The address stack base and size of the stack do not get
set up in the TCB on a system call so creating a frame with
arm_stackframe() would not work.

Am I missing something?

On 4/26/2023 2:14 PM, Ville Juven wrote:
Hi,

But, why need involve kheap here? If the security is your concern, you
should enable per-thread kernel stack in the first place. It's safe to
allocate waitlist/holder from this stack directly, since userspace code
can't touch this area too.
Not from a security perspective, and of course I have the per-thread
kstack
enabled (kernel mode won't work without it) but how do you keep the
frame
from being corrupted / freed until it is not needed any more? Do you
suggest the frame is allocated for each waiting thread separately ? How
does that work ?

I was thinking the first one who waits in sem_wait() allocates the
kernel
structure, puts a reference to it into e.g. the user semaphore, so other
waiters and the kernel itself can find it later (reference being ID, not
pointer!).

But, which scenario needs to access kernel struct through user
semaphore?
It is not necessary to make the access via user semaphore, but
somehow the
kernel side structure needs to be tied to the semaphore, so the
kernel can
find it. Linux does this by a hash function, if I remember correctly.
I'm
not sure duplicating this is a good approach, my concern is the
real-time
aspect.

sem_waitirq() is the problem I have, and fixing it is nasty. The
semaphore
is accessed via tcb->waitobj and this access can happen from
interrupt or
signal dispatch. This means the context of sem_wait() / sem_post() is
_not_
the only place the access happens. sem_waitirq() currently also modifies
the semaphore counter value, so it needs access to the userspace
counter.
This I will do by mapping the page where the counter exists to kernel
(implement something like kmap() / vmap()).

If you have an alternative suggestion on how to fix sem_waitirq() I will
gladly implement it.

Br,
Ville Juven

On Wed, Apr 26, 2023 at 10:34 PM Xiang Xiao <xiaoxiang781...@gmail.com>
wrote:

On Thu, Apr 27, 2023 at 2:18 AM Ville Juven <ville.ju...@gmail.com>
wrote:

Hi,

Yes exactly, this is the basic idea. Like I said, the waitlist/holder
structure is needed only when sem_wait needs to block / wait for the
semaphore to become available.

How to protect the integrity of the stack allocated structure is
still a
bit open but one option is to use kheap as well. Semantics to be
figured
out, the solution should be feasible.

But, why need involve kheap here? If the security is your concern, you
should enable per-thread kernel stack in the first place. It's safe to
allocate waitlist/holder from this stack directly, since userspace code
can't touch this area too.


My idea was to put the handle to this data into the user semaphore,
however
a pointer must not be used, a handle / integer id is needed, which
then
holds the pointer (much like files etc). As the user can spoof /
destroy
the pointer it is unsafe to do that. Spoofing the id can cause the
user
process to crash, but the kernel integrity remains.


But, which scenario needs to access kernel struct through user
semaphore?


Br,
Ville Juven

On Wed, Apr 26, 2023 at 9:09 PM Xiang Xiao <xiaoxiang781...@gmail.com
wrote:

But why not allocate list entry and holder temporally inside the
kernel
thread stack? Both data is just used when the calling thread can't
hold,
but wait the semphare, this trick is widely used inside Linux kernel.

On Thu, Apr 27, 2023 at 1:32 AM Ville Juven <ville.ju...@gmail.com>
wrote:
Hi,

Thanks for the explanation, it makes sense. I had an idea to move
struct
sem_s entirely to kernel and make the user use it via a handle (in
KERNEL
build) but due to the ubiquitous init macros, that is not a feasible
solution.

I will explore allocating the kernel structures on-demand next.

What does this mean ? The kernel side structures (dq_waitlist and
priority
inheritance holders) are only needed when the semaphore is locked
and
someone is actually waiting for it to be released. This means that
when
sem_wait() results in the thread getting blocked, the kernel part
can
be
allocated here and a reference placed into the user sem_t structure.
When
the semaphore unlocks, the kernel parts can be freed.

This should be completely regression free for FLAT/PROTECTED, which
is
very
important for me because I don't want to be responsible for breaking
such a
fundamental OS API for current users ;)

Br,
Ville

On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt <spudan...@gmail.com>
wrote:
I have a question about using mutex_t / struct mutex_s inside
libs/libc. The mutex_t definition is kernel internal but some
modules
inside the libs folder use it directly. My question is, is this
intentional ? I don't think such modules should be using mutex_t.

...

My question ? Should these be sem_t or perhaps pthread_mutex_t ?
Both
of which are valid user APIs.
It was sem_t before it was changed to the non-standard mutex_t. See
commit d1d46335df6:

commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d
Author: anjiahao <anjia...@xiaomi.com>
Date:   Tue Sep 6 14:18:45 2022 +0800

       Replace nxsem API when used as a lock with nxmutex API

       Signed-off-by: anjiahao <anjia...@xiaomi.com>
       Signed-off-by: Xiang Xiao <xiaoxi...@xiaomi.com>

Parts of the above which introduce inappropriate use of mutex_t
could
be
reverted back to sem_t.  Any use of any non-standard OS function or
structure should be prohibited in application code.

However, nuttx/libs is a special creature.  It is not confined by
the
rules of POSIX interface because user-space libraries are a
non-privileged extension of the operating system. They have
intimate
knowledge of OS internals and can do non-standard things to
accomplish
what they need to do.  So although I think it is wrong
architecturally
for applications to use OS internals, I don't think it is wrong for
nuttx/libs to do so.  It is a friend of the OS.

But it is should not break the KERNEL build either.

Perhaps we will need a user-space function to allocate (and
initialize)
kernel memory.  Use of such functions should not performed outside
of
nuttx/libs, although we have no way to prohibit that use:  Such an
API
would be a security hole. user-space code cannot access kernel
memory,
but it can use the kernel memory address like a "handle" to
interact
with the OS.