| Message ID | 20170106162116.12859-1-kirkseraph@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCHv2] target-arm/abi32: check for segfault in do_kernel_trap
Message-id: 20170106162116.12859-1-kirkseraph@gmail.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20170106162116.12859-1-kirkseraph@gmail.com -> patchew/20170106162116.12859-1-kirkseraph@gmail.com
Switched to a new branch 'test'
8ea3062 target-arm/abi32: check for segfault in do_kernel_trap
=== OUTPUT BEGIN ===
Checking PATCH 1/1: target-arm/abi32: check for segfault in do_kernel_trap...
ERROR: braces {} are necessary for all arms of this statement
#56: FILE: linux-user/main.c:517:
+ if (get_user_u32(val, addr))
[...]
ERROR: braces {} are necessary for all arms of this statement
#60: FILE: linux-user/main.c:521:
+ if (put_user_u32(val, addr))
[...]
total: 2 errors, 0 warnings, 95 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 6 January 2017 at 16:21, Seraphime Kirkovski <kirkseraph@gmail.com> wrote: > Currently, the cmpxchg implementation tests whether the destination address > is readable: > - if it is, we read the value and continue with the comparison > - if isn't, i.e. access to addr would segfault, we assume that src != dest > rather than queuing a SIGSEGV. > > The same problem exists in the case where src == dest: the code doesn't > check whether put_user_u32 succeeds. > > This fixes both problems by sending a SIGSEGV when the destination address > is inaccessible. > > Signed-off-by: Seraphime Kirkovski <kirkseraph@gmail.com> > --- > > This accounts for Peter Maydell's remarks. > The refactoring here extracts the cmpxchg code from do_kernel_trap in its own > function. As the patchew robot notes, our coding style wants braces on all if() statements, even one-line ones. Other than that, Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/linux-user/main.c b/linux-user/main.c index c1d5eb4d6f..ed38ebe0c1 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -441,7 +441,7 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env) uint32_t addr, cpsr; target_siginfo_t info; - /* Based on the 32 bit code in do_kernel_trap */ + /* Based on the 32 bit code in arm_kernel_cmpxchg_helper */ /* XXX: This only works between threads, not between processes. It's probably possible to implement this with native host @@ -496,41 +496,63 @@ segv: queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); } +/* + * 32bit cmpxchg kernel user helper. + * See the note above arm_kernel_cmpxchg64_helper. + */ +static void arm_kernel_cmpxchg_helper(CPUARMState *env) +{ + uint32_t addr; + uint32_t cpsr; + uint32_t val; + target_siginfo_t info; + + /* XXX: This only works between threads, not between processes. + It's probably possible to implement this with native host + operations. However things like ldrex/strex are much harder so + there's not much point trying. */ + start_exclusive(); + cpsr = cpsr_read(env); + addr = env->regs[2]; + if (get_user_u32(val, addr)) + goto segv; + if (val == env->regs[0]) { + val = env->regs[1]; + if (put_user_u32(val, addr)) + goto segv; + env->regs[0] = 0; + cpsr |= CPSR_C; + } else { + env->regs[0] = -1; + cpsr &= ~CPSR_C; + } + cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr); + end_exclusive(); + return; + +segv: + end_exclusive(); + env->exception.vaddress = addr; + info.si_signo = TARGET_SIGSEGV; + info.si_errno = 0; + /* XXX: check env->error_code */ + info.si_code = TARGET_SEGV_MAPERR; + info._sifields._sigfault._addr = env->exception.vaddress; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); +} + /* Handle a jump to the kernel code page. */ static int do_kernel_trap(CPUARMState *env) { uint32_t addr; - uint32_t cpsr; - uint32_t val; switch (env->regs[15]) { case 0xffff0fa0: /* __kernel_memory_barrier */ /* ??? No-op. Will need to do better for SMP. */ break; case 0xffff0fc0: /* __kernel_cmpxchg */ - /* XXX: This only works between threads, not between processes. - It's probably possible to implement this with native host - operations. However things like ldrex/strex are much harder so - there's not much point trying. */ - start_exclusive(); - cpsr = cpsr_read(env); - addr = env->regs[2]; - /* FIXME: This should SEGV if the access fails. */ - if (get_user_u32(val, addr)) - val = ~env->regs[0]; - if (val == env->regs[0]) { - val = env->regs[1]; - /* FIXME: Check for segfaults. */ - put_user_u32(val, addr); - env->regs[0] = 0; - cpsr |= CPSR_C; - } else { - env->regs[0] = -1; - cpsr &= ~CPSR_C; - } - cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr); - end_exclusive(); + arm_kernel_cmpxchg_helper(env); break; case 0xffff0fe0: /* __kernel_get_tls */ env->regs[0] = cpu_get_tls(env);
Currently, the cmpxchg implementation tests whether the destination address is readable: - if it is, we read the value and continue with the comparison - if isn't, i.e. access to addr would segfault, we assume that src != dest rather than queuing a SIGSEGV. The same problem exists in the case where src == dest: the code doesn't check whether put_user_u32 succeeds. This fixes both problems by sending a SIGSEGV when the destination address is inaccessible. Signed-off-by: Seraphime Kirkovski <kirkseraph@gmail.com> --- This accounts for Peter Maydell's remarks. The refactoring here extracts the cmpxchg code from do_kernel_trap in its own function. linux-user/main.c | 72 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 25 deletions(-)