Haren Myneni's on March 18, 2020 5:27 am:
> On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
>> Haren Myneni <ha...@linux.ibm.com> writes:
>> > For each fault CRB, update fault address in CRB (fault_storage_addr)
>> > and translation error status in CSB so that user space can touch the
>> > fault address and resend the request. If the user space passed invalid
>> > CSB address send signal to process with SIGSEGV.
>> >
>> > Signed-off-by: Sukadev Bhattiprolu <suka...@linux.vnet.ibm.com>
>> > Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
>> > ---
>> > arch/powerpc/platforms/powernv/vas-fault.c | 114
>> > +++++++++++++++++++++++++++++
>> > 1 file changed, 114 insertions(+)
>> >
>> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c
>> > b/arch/powerpc/platforms/powernv/vas-fault.c
>> > index 1c6d5cc..751ce48 100644
>> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
>> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
>> > @@ -11,6 +11,7 @@
>> > #include <linux/slab.h>
>> > #include <linux/uaccess.h>
>> > #include <linux/kthread.h>
>> > +#include <linux/sched/signal.h>
>> > #include <linux/mmu_context.h>
>> > #include <asm/icswx.h>
>> >
>> > @@ -26,6 +27,118 @@
>> > #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
>> >
>> > /*
>> > + * Update the CSB to indicate a translation error.
>> > + *
>> > + * If we are unable to update the CSB means copy_to_user failed due to
>> > + * invalid csb_addr, send a signal to the process.
>> > + *
>> > + * Remaining settings in the CSB are based on wait_for_csb() of
>> > + * NX-GZIP.
>> > + */
>> > +static void update_csb(struct vas_window *window,
>> > + struct coprocessor_request_block *crb)
>> > +{
>> > + int rc;
>> > + struct pid *pid;
>> > + void __user *csb_addr;
>> > + struct task_struct *tsk;
>> > + struct kernel_siginfo info;
>> > + struct coprocessor_status_block csb;
>>
>> csb is on the stack, and later copied to user, which is a risk for
>> creating an infoleak.
>>
>> Also please use reverse Christmas tree layout for your variables.
>>
>> > +
>> > + /*
>> > + * NX user space windows can not be opened for task->mm=NULL
>> > + * and faults will not be generated for kernel requests.
>> > + */
>> > + if (!window->mm || !window->user_win)
>> > + return;
>>
>> If that's a should-never-happen condition then should it do a
>> WARN_ON_ONCE() rather than silently returning?
>
> Will add WARN_ON
>
>>
>> > + csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
>> > +
>> > + csb.cc = CSB_CC_TRANSLATION;
>> > + csb.ce = CSB_CE_TERMINATION;
>> > + csb.cs = 0;
>> > + csb.count = 0;
>> > +
>> > + /*
>> > + * NX operates and returns in BE format as defined CRB struct.
>> > + * So return fault_storage_addr in BE as NX pastes in FIFO and
>> > + * expects user space to convert to CPU format.
>> > + */
>> > + csb.address = crb->stamp.nx.fault_storage_addr;
>> > + csb.flags = 0;
>>
>> I'm pretty sure this has initialised all the fields of csb.
>>
>> But, I'd still be much happier if you zeroed the whole struct to begin
>> with, that way we know for sure we can't leak any uninitialised bytes to
>> userspace. It's only 16 bytes so it shouldn't add any noticeable
>> overhead.
> Sure, will initialize csb
>>
>> > +
>> > + pid = window->pid;
>> > + tsk = get_pid_task(pid, PIDTYPE_PID);
>> > + /*
>> > + * Send window will be closed after processing all NX requests
>> > + * and process exits after closing all windows. In multi-thread
>> > + * applications, thread may not exists, but does not close FD
>> > + * (means send window) upon exit. Parent thread (tgid) can use
>> > + * and close the window later.
>> > + * pid and mm references are taken when window is opened by
>> > + * process (pid). So tgid is used only when child thread opens
>> > + * a window and exits without closing it in multithread tasks.
>> > + */
>> > + if (!tsk) {
>> > + pid = window->tgid;
>> > + tsk = get_pid_task(pid, PIDTYPE_PID);
>> > + /*
>> > + * Parent thread will be closing window during its exit.
>> > + * So should not get here.
>> > + */
>> > + if (!tsk)
>> > + return;
>>
>> Similar question on WARN_ON_ONCE()
> Yes, we can add WARN_ON
>>
>> > + }
>> > +
>> > + /* Return if the task is exiting. */
>>
>> Why? Just because it's no use? It's racy isn't it, so it can't be for
>> correctness?
> Yes process is exiting and no need to update CSB. We release the
> task->usage refcount after copy_to_user().
>
>>
>> > + if (tsk->flags & PF_EXITING) {
>> > + put_task_struct(tsk);
>> > + return;
>> > + }
>> > +
>> > + use_mm(window->mm);
>>
>> There's no check that csb_addr is actually pointing into userspace, but
>> copy_to_user() does it for you.
>>
>> > + rc = copy_to_user(csb_addr, &csb, sizeof(csb));
>> > + /*
>> > + * User space polls on csb.flags (first byte). So add barrier
>> > + * then copy first byte with csb flags update.
>> > + */
>> > + smp_mb();
>>
>> You only need to order the stores above vs the store below to csb.flags.
>> So you should only need an smp_wmb() here.
> Sure, will add
> if (!rc) {
> csb.flags = CSB_V;
> smp_mb();
> rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> }
>
>>
>> > + if (!rc) {
>> > + csb.flags = CSB_V;
>> > + rc = copy_to_user(csb_addr, &csb, sizeof(u8));
>> > + }
>> > + unuse_mm(window->mm);
>> > + put_task_struct(tsk);
>> > +
>> > + /* Success */
>> > + if (!rc)
>> > + return;
>> > +
>> > + pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
>> > + csb_addr, pid_vnr(pid));
>> > +
>> > + clear_siginfo(&info);
>> > + info.si_signo = SIGSEGV;
>> > + info.si_errno = EFAULT;
>> > + info.si_code = SEGV_MAPERR;
>> > + info.si_addr = csb_addr;
>> > +
>> > + /*
>> > + * process will be polling on csb.flags after request is sent to
>> > + * NX. So generally CSB update should not fail except when an
>> > + * application does not follow the process properly. So an error
>> > + * message will be displayed and leave it to user space whether
>> > + * to ignore or handle this signal.
>> > + */
The code would read a bit better if this comment goes at the start of
this error handling process it describes (before the error message).
And I feel a bit hypocritical complaining about readability, but it
could possibly do with some work.
/*
* The application should have provided a valid mapping for the
* csb, and not unmap it before the csb.flags update, so the
* copy_to_user should not fail.
*
* If the application fails to follow this protocol, log a kernel
* error and send a SEGV to the pid. This signal may be ignored,
* so can't use force_sig_fault_to_task()
*/
Something like tthat?
>> > + rcu_read_lock();
>> > + rc = kill_pid_info(SIGSEGV, &info, pid);
>> > + rcu_read_unlock();
ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
it's held around kill_pid_info in kernel/signal.c for the find_vpid().
Thanks,
Nick
>>
>> Shouldn't this be using force_sig_fault_to_task() or another helper,
>> rather than open-coding?
>
> Applications or nxz library can ignore this signal based on si_addr or
> take action like resend new request with valid csb_addr. Hence I did not
> use force_sig_info_to_task().
>
>>
>> > +
>> > + pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
>> > + pid_vnr(pid), rc);
>> > +}
>> > +
>> > +/*
>> > * Process valid CRBs in fault FIFO.
>> > */
>> > irqreturn_t vas_fault_thread_fn(int irq, void *data)
>> > @@ -111,6 +224,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>> > return IRQ_HANDLED;
>> > }
>> >
>> > + update_csb(window, crb);
>> > } while (true);
>> > }
>> >
>> > --
>> > 1.8.3.1
>>
>> cheers
>
>
>
