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.
> >>>>>>>
> >>>>>>>
> >>>>>>>
>
