In 64bit architecture, sigval_int is the high 32bit of sigval_ptr in
big endian kernel compare with low 32bit of sigval_ptr in little
endian kernel. reference:
typedef union sigval {
int sival_int;
void *sival_ptr;
} sigval_t;
During compat_mq_notify or compat_timer_create, kernel get sigval
from user space by reading sigval.sival_int. This is correct in 32 bit
kernel and in 64bit little endian kernel. And It is wrong in 64bit big
endian kernel:
It get the high 32bit of sigval_ptr and put it to low 32bit of
sigval_ptr. And the high 32bit sigval_ptr in empty in arm 32bit user
space struct. So, kernel lost the value of sigval_ptr.
The following patch get the sigval_ptr in stead of sigval_int in order
to avoid endian issue.
Test pass in arm64 big endian and little endian kernel.
Signed-off-by: Zhang Jian(Bamvor) <[email protected]>
---
ipc/compat_mq.c | 7 ++-----
kernel/compat.c | 6 ++----
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/ipc/compat_mq.c b/ipc/compat_mq.c
index ef6f91c..2e07343 100644
--- a/ipc/compat_mq.c
+++ b/ipc/compat_mq.c
@@ -99,11 +99,8 @@ COMPAT_SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
if (u_notification) {
struct sigevent n;
p = compat_alloc_user_space(sizeof(*p));
- if (get_compat_sigevent(&n, u_notification))
- return -EFAULT;
- if (n.sigev_notify == SIGEV_THREAD)
- n.sigev_value.sival_ptr = compat_ptr(n.sigev_value.sival_int);
- if (copy_to_user(p, &n, sizeof(*p)))
+ if (get_compat_sigevent(&n, u_notification) ||
+ copy_to_user(p, &n, sizeof(*p)))
return -EFAULT;
}
return sys_mq_notify(mqdes, p);
diff --git a/kernel/compat.c b/kernel/compat.c
index ebb3c36..13a0e5d 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -871,16 +871,14 @@ COMPAT_SYSCALL_DEFINE4(clock_nanosleep, clockid_t, which_clock, int, flags,
* We currently only need the following fields from the sigevent
* structure: sigev_value, sigev_signo, sig_notify and (sometimes
* sigev_notify_thread_id). The others are handled in user mode.
- * We also assume that copying sigev_value.sival_int is sufficient
- * to keep all the bits of sigev_value.sival_ptr intact.
*/
int get_compat_sigevent(struct sigevent *event,
const struct compat_sigevent __user *u_event)
{
memset(event, 0, sizeof(*event));
return (!access_ok(VERIFY_READ, u_event, sizeof(*u_event)) ||
- __get_user(event->sigev_value.sival_int,
- &u_event->sigev_value.sival_int) ||
+ __get_user(*(long long*)event->sigev_value.sival_ptr,
+ &u_event->sigev_value.sival_ptr) ||
__get_user(event->sigev_signo, &u_event->sigev_signo) ||
__get_user(event->sigev_notify, &u_event->sigev_notify) ||
__get_user(event->sigev_notify_thread_id,
--
1.8.4.5
cc'ing linux-arch as well.
On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
> In 64bit architecture, sigval_int is the high 32bit of sigval_ptr in
> big endian kernel compare with low 32bit of sigval_ptr in little
> endian kernel. reference:
>
> typedef union sigval {
> int sival_int;
> void *sival_ptr;
> } sigval_t;
>
> During compat_mq_notify or compat_timer_create, kernel get sigval
> from user space by reading sigval.sival_int. This is correct in 32 bit
> kernel and in 64bit little endian kernel. And It is wrong in 64bit big
> endian kernel:
> It get the high 32bit of sigval_ptr and put it to low 32bit of
> sigval_ptr. And the high 32bit sigval_ptr in empty in arm 32bit user
> space struct. So, kernel lost the value of sigval_ptr.
>
> The following patch get the sigval_ptr in stead of sigval_int in order
> to avoid endian issue.
> Test pass in arm64 big endian and little endian kernel.
>
> Signed-off-by: Zhang Jian(Bamvor) <[email protected]>
> ---
> ipc/compat_mq.c | 7 ++-----
> kernel/compat.c | 6 ++----
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/ipc/compat_mq.c b/ipc/compat_mq.c
> index ef6f91c..2e07343 100644
> --- a/ipc/compat_mq.c
> +++ b/ipc/compat_mq.c
> @@ -99,11 +99,8 @@ COMPAT_SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
> if (u_notification) {
> struct sigevent n;
> p = compat_alloc_user_space(sizeof(*p));
> - if (get_compat_sigevent(&n, u_notification))
> - return -EFAULT;
> - if (n.sigev_notify == SIGEV_THREAD)
> - n.sigev_value.sival_ptr = compat_ptr(n.sigev_value.sival_int);
> - if (copy_to_user(p, &n, sizeof(*p)))
> + if (get_compat_sigevent(&n, u_notification) ||
> + copy_to_user(p, &n, sizeof(*p)))
> return -EFAULT;
The kernel doesn't need to interpret the sival_ptr value, it's something
to be passed to the notifier function as an argument. For the user's
convenience, it is a union of an int and ptr. So I think the original
SIGEV_THREAD check and compat_ptr() conversion here is wrong, it
shouldn't exist at all (it doesn't exist in compat_sys_timer_create
either). This hunk looks fine to me.
> }
> return sys_mq_notify(mqdes, p);
> diff --git a/kernel/compat.c b/kernel/compat.c
> index ebb3c36..13a0e5d 100644
> --- a/kernel/compat.c
> +++ b/kernel/compat.c
> @@ -871,16 +871,14 @@ COMPAT_SYSCALL_DEFINE4(clock_nanosleep, clockid_t, which_clock, int, flags,
> * We currently only need the following fields from the sigevent
> * structure: sigev_value, sigev_signo, sig_notify and (sometimes
> * sigev_notify_thread_id). The others are handled in user mode.
> - * We also assume that copying sigev_value.sival_int is sufficient
> - * to keep all the bits of sigev_value.sival_ptr intact.
> */
> int get_compat_sigevent(struct sigevent *event,
> const struct compat_sigevent __user *u_event)
> {
> memset(event, 0, sizeof(*event));
> return (!access_ok(VERIFY_READ, u_event, sizeof(*u_event)) ||
> - __get_user(event->sigev_value.sival_int,
> - &u_event->sigev_value.sival_int) ||
> + __get_user(*(long long*)event->sigev_value.sival_ptr,
> + &u_event->sigev_value.sival_ptr) ||
I don't think this is correct because some (most) architectures use
sival_int here when copying to user and for a big endian compat they
would get 0 for sival_int (mips, powerpc).
I think the correct fix is in the arm64 code:
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index e299de396e9b..32601939a3c8 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
case __SI_TIMER:
err |= __put_user(from->si_tid, &to->si_tid);
err |= __put_user(from->si_overrun, &to->si_overrun);
- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
- &to->si_ptr);
+ err |= __put_user(from->si_int, &to->si_int);
break;
case __SI_POLL:
err |= __put_user(from->si_band, &to->si_band);
@@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
case __SI_MESGQ: /* But this is */
err |= __put_user(from->si_pid, &to->si_pid);
err |= __put_user(from->si_uid, &to->si_uid);
- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
+ err |= __put_user(from->si_int, &to->si_int);
break;
case __SI_SYS:
err |= __put_user((compat_uptr_t)(unsigned long)
But we need to check other architectures that are capable of big endian
and have a compat layer.
--
Catalin
On 2015/2/10 20:27, Catalin Marinas wrote:
> cc'ing linux-arch as well.
>
> On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
>> In 64bit architecture, sigval_int is the high 32bit of sigval_ptr in
>> big endian kernel compare with low 32bit of sigval_ptr in little
>> endian kernel. reference:
>>
>> typedef union sigval {
>> int sival_int;
>> void *sival_ptr;
>> } sigval_t;
>>
>> During compat_mq_notify or compat_timer_create, kernel get sigval
>> from user space by reading sigval.sival_int. This is correct in 32 bit
>> kernel and in 64bit little endian kernel. And It is wrong in 64bit big
>> endian kernel:
>> It get the high 32bit of sigval_ptr and put it to low 32bit of
>> sigval_ptr. And the high 32bit sigval_ptr in empty in arm 32bit user
>> space struct. So, kernel lost the value of sigval_ptr.
>>
>> The following patch get the sigval_ptr in stead of sigval_int in order
>> to avoid endian issue.
>> Test pass in arm64 big endian and little endian kernel.
>>
>> Signed-off-by: Zhang Jian(Bamvor) <[email protected]>
>> ---
>> ipc/compat_mq.c | 7 ++-----
>> kernel/compat.c | 6 ++----
>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/ipc/compat_mq.c b/ipc/compat_mq.c
>> index ef6f91c..2e07343 100644
>> --- a/ipc/compat_mq.c
>> +++ b/ipc/compat_mq.c
>> @@ -99,11 +99,8 @@ COMPAT_SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
>> if (u_notification) {
>> struct sigevent n;
>> p = compat_alloc_user_space(sizeof(*p));
>> - if (get_compat_sigevent(&n, u_notification))
>> - return -EFAULT;
>> - if (n.sigev_notify == SIGEV_THREAD)
>> - n.sigev_value.sival_ptr = compat_ptr(n.sigev_value.sival_int);
>> - if (copy_to_user(p, &n, sizeof(*p)))
>> + if (get_compat_sigevent(&n, u_notification) ||
>> + copy_to_user(p, &n, sizeof(*p)))
>> return -EFAULT;
>
> The kernel doesn't need to interpret the sival_ptr value, it's something
> to be passed to the notifier function as an argument.
Yeah, this is the reason why I try to fix sival_ptr through
get_compat_sigevent before sys_mq_notify. After this compat wrapper,
sys_mq_notify will put the sival_ptr to nc buffer(file ipc/mqueue.c line 1221
to line 1226):
if (copy_from_user(nc->data,
notification.sigev_value.sival_ptr,
NOTIFY_COOKIE_LEN)) {
ret = -EFAULT;
goto out;
}
/* TODO: add a header? */
skb_put(nc, NOTIFY_COOKIE_LEN);
/* and attach it to the socket */
The original changes is introduced by Alexander Viro
<[email protected]> more than ten years ago[1]:
author Alexander Viro <[email protected]> 2004-07-13 04:02:57 (GMT)
committer Linus Torvalds <[email protected]> 2004-07-13 04:02:57 (GMT)
commit 95b5842264ac470a1a3a59d2741bb18adb140c8b (patch)
tree 5167f68fae8f3bbc9b3a2d7617d1500356837c16 /ipc/compat_mq.c
parent de54add33621c5b4a1be895c82b7af96fb4dd447 (diff)
[PATCH] sparse: ipc compat annotations and cleanups
ipc compat code switched to compat_alloc_user_space() and annotated.
> For the user's
> convenience, it is a union of an int and ptr. So I think the original
> SIGEV_THREAD check and compat_ptr() conversion here is wrong, it
> shouldn't exist at all (it doesn't exist in compat_sys_timer_create
> either). This hunk looks fine to me.
>
>> }
>> return sys_mq_notify(mqdes, p);
>> diff --git a/kernel/compat.c b/kernel/compat.c
>> index ebb3c36..13a0e5d 100644
>> --- a/kernel/compat.c
>> +++ b/kernel/compat.c
>> @@ -871,16 +871,14 @@ COMPAT_SYSCALL_DEFINE4(clock_nanosleep, clockid_t, which_clock, int, flags,
>> * We currently only need the following fields from the sigevent
>> * structure: sigev_value, sigev_signo, sig_notify and (sometimes
>> * sigev_notify_thread_id). The others are handled in user mode.
>> - * We also assume that copying sigev_value.sival_int is sufficient
>> - * to keep all the bits of sigev_value.sival_ptr intact.
>> */
>> int get_compat_sigevent(struct sigevent *event,
>> const struct compat_sigevent __user *u_event)
>> {
>> memset(event, 0, sizeof(*event));
>> return (!access_ok(VERIFY_READ, u_event, sizeof(*u_event)) ||
>> - __get_user(event->sigev_value.sival_int,
>> - &u_event->sigev_value.sival_int) ||
>> + __get_user(*(long long*)event->sigev_value.sival_ptr,
should be:
__get_user(event->sigev_value.sival_ptr,
> > + &u_event->sigev_value.sival_ptr) ||
>
> I don't think this is correct because some (most) architectures use
> sival_int here when copying to user and for a big endian compat they
> would get 0 for sival_int (mips, powerpc).
Sorry, I am lost here. As we mentioned above, sival_ptr and sival_int
is union, so, copy sival_ptr should include sival_int.
>
> I think the correct fix is in the arm64 code:
The following code could fix my issue.
regards
bamvor
> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> index e299de396e9b..32601939a3c8 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> case __SI_TIMER:
> err |= __put_user(from->si_tid, &to->si_tid);
> err |= __put_user(from->si_overrun, &to->si_overrun);
> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
> - &to->si_ptr);
> + err |= __put_user(from->si_int, &to->si_int);
> break;
> case __SI_POLL:
> err |= __put_user(from->si_band, &to->si_band);
> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> case __SI_MESGQ: /* But this is */
> err |= __put_user(from->si_pid, &to->si_pid);
> err |= __put_user(from->si_uid, &to->si_uid);
> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
> + err |= __put_user(from->si_int, &to->si_int);
> break;
> case __SI_SYS:
> err |= __put_user((compat_uptr_t)(unsigned long)
>
> But we need to check other architectures that are capable of big endian
> and have a compat layer.
>
On Wed, Feb 11, 2015 at 07:22:08PM +0800, Bamvor Jian Zhang wrote:
> On 2015/2/10 20:27, Catalin Marinas wrote:
> > On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
> >> In 64bit architecture, sigval_int is the high 32bit of sigval_ptr in
> >> big endian kernel compare with low 32bit of sigval_ptr in little
> >> endian kernel. reference:
> >>
> >> typedef union sigval {
> >> int sival_int;
> >> void *sival_ptr;
> >> } sigval_t;
> >>
> >> During compat_mq_notify or compat_timer_create, kernel get sigval
> >> from user space by reading sigval.sival_int. This is correct in 32 bit
> >> kernel and in 64bit little endian kernel. And It is wrong in 64bit big
> >> endian kernel:
> >> It get the high 32bit of sigval_ptr and put it to low 32bit of
> >> sigval_ptr. And the high 32bit sigval_ptr in empty in arm 32bit user
> >> space struct. So, kernel lost the value of sigval_ptr.
> >>
> >> The following patch get the sigval_ptr in stead of sigval_int in order
> >> to avoid endian issue.
> >> Test pass in arm64 big endian and little endian kernel.
> >>
> >> Signed-off-by: Zhang Jian(Bamvor) <[email protected]>
> >> ---
> >> ipc/compat_mq.c | 7 ++-----
> >> kernel/compat.c | 6 ++----
> >> 2 files changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/ipc/compat_mq.c b/ipc/compat_mq.c
> >> index ef6f91c..2e07343 100644
> >> --- a/ipc/compat_mq.c
> >> +++ b/ipc/compat_mq.c
> >> @@ -99,11 +99,8 @@ COMPAT_SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
> >> if (u_notification) {
> >> struct sigevent n;
> >> p = compat_alloc_user_space(sizeof(*p));
> >> - if (get_compat_sigevent(&n, u_notification))
> >> - return -EFAULT;
> >> - if (n.sigev_notify == SIGEV_THREAD)
> >> - n.sigev_value.sival_ptr = compat_ptr(n.sigev_value.sival_int);
> >> - if (copy_to_user(p, &n, sizeof(*p)))
> >> + if (get_compat_sigevent(&n, u_notification) ||
> >> + copy_to_user(p, &n, sizeof(*p)))
> >> return -EFAULT;
> >
> > The kernel doesn't need to interpret the sival_ptr value, it's something
> > to be passed to the notifier function as an argument.
Actually I was wrong here. The kernel _does_ interpret the sival_ptr to
read a cookie in sys_mq_notify() for PF_NETLINK sockets. So this code is
correct.
> >> }
> >> return sys_mq_notify(mqdes, p);
> >> diff --git a/kernel/compat.c b/kernel/compat.c
> >> index ebb3c36..13a0e5d 100644
> >> --- a/kernel/compat.c
> >> +++ b/kernel/compat.c
> >> @@ -871,16 +871,14 @@ COMPAT_SYSCALL_DEFINE4(clock_nanosleep, clockid_t, which_clock, int, flags,
> >> * We currently only need the following fields from the sigevent
> >> * structure: sigev_value, sigev_signo, sig_notify and (sometimes
> >> * sigev_notify_thread_id). The others are handled in user mode.
> >> - * We also assume that copying sigev_value.sival_int is sufficient
> >> - * to keep all the bits of sigev_value.sival_ptr intact.
> >> */
> >> int get_compat_sigevent(struct sigevent *event,
> >> const struct compat_sigevent __user *u_event)
> >> {
> >> memset(event, 0, sizeof(*event));
> >> return (!access_ok(VERIFY_READ, u_event, sizeof(*u_event)) ||
> >> - __get_user(event->sigev_value.sival_int,
> >> - &u_event->sigev_value.sival_int) ||
> >> + __get_user(*(long long*)event->sigev_value.sival_ptr,
>
> should be:
> __get_user(event->sigev_value.sival_ptr,
>
> > > + &u_event->sigev_value.sival_ptr) ||
> >
> > I don't think this is correct because some (most) architectures use
> > sival_int here when copying to user and for a big endian compat they
> > would get 0 for sival_int (mips, powerpc).
>
> Sorry, I am lost here. As we mentioned above, sival_ptr and sival_int
> is union, so, copy sival_ptr should include sival_int.
The user access size in your patch is the size of sival_ptr which is
32-bit for compat, same as sival_int; so far, so good. However, when you
store it to the native sival_ptr, you perform a conversion to long long
(shouldn't it be unsigned long long, or just unsigned long?).
The native sigval_t is also a union but on 64-bit big endian, the
sival_int overlaps with the most significant 32-bit of the sival_ptr.
So reading sival_int would always be 0. When the compat siginfo is
copied to user, arm64 reads the native sival_ptr (si_ptr) and converts
it to the compat one, getting the correct 32-bit value. However, other
architectures access sival_int (si_int) instead which breaks with your
get_compat_sigevent() changes.
> > I think the correct fix is in the arm64 code:
>
> The following code could fix my issue.
Without any parts of your patch?
I think that's correct fix since in the SIGEV_THREAD mq_notify case, we
would not deliver a signal as notification, so the sival_int value is
irrelevant (it would be 0 for big-endian compat tasks because of the
sigval_t union on 64-bit).
Your patch would work as well but you have to change all the other
architectures to use si_ptr when copying to a compat siginfo.
> > diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> > index e299de396e9b..32601939a3c8 100644
> > --- a/arch/arm64/kernel/signal32.c
> > +++ b/arch/arm64/kernel/signal32.c
> > @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> > case __SI_TIMER:
> > err |= __put_user(from->si_tid, &to->si_tid);
> > err |= __put_user(from->si_overrun, &to->si_overrun);
> > - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
> > - &to->si_ptr);
> > + err |= __put_user(from->si_int, &to->si_int);
> > break;
> > case __SI_POLL:
> > err |= __put_user(from->si_band, &to->si_band);
> > @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> > case __SI_MESGQ: /* But this is */
> > err |= __put_user(from->si_pid, &to->si_pid);
> > err |= __put_user(from->si_uid, &to->si_uid);
> > - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
> > + err |= __put_user(from->si_int, &to->si_int);
> > break;
> > case __SI_SYS:
> > err |= __put_user((compat_uptr_t)(unsigned long)
--
Catalin
On 2015/2/11 23:40, Catalin Marinas wrote:
> On Wed, Feb 11, 2015 at 07:22:08PM +0800, Bamvor Jian Zhang wrote:
>> On 2015/2/10 20:27, Catalin Marinas wrote:
>>> On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
...
> The native sigval_t is also a union but on 64-bit big endian, the
> sival_int overlaps with the most significant 32-bit of the sival_ptr.
> So reading sival_int would always be 0. When the compat siginfo is
> copied to user, arm64 reads the native sival_ptr (si_ptr) and converts
> it to the compat one, getting the correct 32-bit value. However, other
> architectures access sival_int (si_int) instead which breaks with your
> get_compat_sigevent() changes.
>
>>> I think the correct fix is in the arm64 code:
>>
>> The following code could fix my issue.
>
> Without any parts of your patch?
Yes. As you mentioned above, sival_int overlaps the most significant 32bit
of the sival_ptr, it seems that your patch is right if sival_ptr is less than
32bit.
> I think that's correct fix since in the SIGEV_THREAD mq_notify case, we
> would not deliver a signal as notification, so the sival_int value is
> irrelevant (it would be 0 for big-endian compat tasks because of the
> sigval_t union on 64-bit).
>
> Your patch would work as well but you have to change all the other
> architectures to use si_ptr when copying to a compat siginfo.
Yeah, it seems that my patch is useful only if the sival_ptr is bigger
than 32bit. It need the similar changes with following catalin's patch
in the following 64bit architecture:
x86: arch/x86/ia32/ia32_signal.c
tile, s390: arch/xxx/kernel/compat_signal.c
parisc, sparc, mips: arch/xxx/kernel/signal32.c
powerpc: arch/xxx/kernel/signal_32.c
cc these maintainers for input.
regards
bamvor
>>> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
>>> index e299de396e9b..32601939a3c8 100644
>>> --- a/arch/arm64/kernel/signal32.c
>>> +++ b/arch/arm64/kernel/signal32.c
>>> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>> case __SI_TIMER:
>>> err |= __put_user(from->si_tid, &to->si_tid);
>>> err |= __put_user(from->si_overrun, &to->si_overrun);
>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
>>> - &to->si_ptr);
>>> + err |= __put_user(from->si_int, &to->si_int);
>>> break;
>>> case __SI_POLL:
>>> err |= __put_user(from->si_band, &to->si_band);
>>> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>> case __SI_MESGQ: /* But this is */
>>> err |= __put_user(from->si_pid, &to->si_pid);
>>> err |= __put_user(from->si_uid, &to->si_uid);
>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
>>> + err |= __put_user(from->si_int, &to->si_int);
>>> break;
>>> case __SI_SYS:
>>> err |= __put_user((compat_uptr_t)(unsigned long)
>
On Fri, Feb 13, 2015 at 04:00:43PM +0800, Bamvor Jian Zhang wrote:
> On 2015/2/11 23:40, Catalin Marinas wrote:
> > On Wed, Feb 11, 2015 at 07:22:08PM +0800, Bamvor Jian Zhang wrote:
> >> On 2015/2/10 20:27, Catalin Marinas wrote:
> >>> On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
> ...
> > The native sigval_t is also a union but on 64-bit big endian, the
> > sival_int overlaps with the most significant 32-bit of the sival_ptr.
> > So reading sival_int would always be 0. When the compat siginfo is
> > copied to user, arm64 reads the native sival_ptr (si_ptr) and converts
> > it to the compat one, getting the correct 32-bit value. However, other
> > architectures access sival_int (si_int) instead which breaks with your
> > get_compat_sigevent() changes.
> >
> >>> I think the correct fix is in the arm64 code:
> >>
> >> The following code could fix my issue.
> >
> > Without any parts of your patch?
>
> Yes. As you mentioned above, sival_int overlaps the most significant 32bit
> of the sival_ptr, it seems that your patch is right if sival_ptr is less than
> 32bit.
I don't follow you here. sival_ptr is greater than 32-bit on a 64-bit
kernel.
> > I think that's correct fix since in the SIGEV_THREAD mq_notify case, we
> > would not deliver a signal as notification, so the sival_int value is
> > irrelevant (it would be 0 for big-endian compat tasks because of the
> > sigval_t union on 64-bit).
> >
> > Your patch would work as well but you have to change all the other
> > architectures to use si_ptr when copying to a compat siginfo.
>
> Yeah, it seems that my patch is useful only if the sival_ptr is bigger
> than 32bit. It need the similar changes with following catalin's patch
> in the following 64bit architecture:
>
> x86: arch/x86/ia32/ia32_signal.c
That's a little endian architecture and the use of ptr_to_compat() looks
fine to me.
> tile, s390: arch/xxx/kernel/compat_signal.c
s390 uses si_int already, as in my proposed arm64 patch.
tile seems to be bi-endian, though I couldn't see a Kconfig option, nor
something defining __BIG_ENDIAN__ in headers or Makefile. I guess it's
coming from the compiler directly. Anyway, on big endian tile, I we have
the same issue as on big endian arm64.
> parisc, sparc, mips: arch/xxx/kernel/signal32.c
paris, sparc and mips all use si_int instead of si_ptr, so that's fine.
> powerpc: arch/xxx/kernel/signal_32.c
Same for powerpc, it uses si_int instead of si_ptr.
> cc these maintainers for input.
I think it's only tile that needs fixing for big endian, something like
the arm64 patch below:
> >>> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> >>> index e299de396e9b..32601939a3c8 100644
> >>> --- a/arch/arm64/kernel/signal32.c
> >>> +++ b/arch/arm64/kernel/signal32.c
> >>> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> >>> case __SI_TIMER:
> >>> err |= __put_user(from->si_tid, &to->si_tid);
> >>> err |= __put_user(from->si_overrun, &to->si_overrun);
> >>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
> >>> - &to->si_ptr);
> >>> + err |= __put_user(from->si_int, &to->si_int);
> >>> break;
> >>> case __SI_POLL:
> >>> err |= __put_user(from->si_band, &to->si_band);
> >>> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> >>> case __SI_MESGQ: /* But this is */
> >>> err |= __put_user(from->si_pid, &to->si_pid);
> >>> err |= __put_user(from->si_uid, &to->si_uid);
> >>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
> >>> + err |= __put_user(from->si_int, &to->si_int);
> >>> break;
> >>> case __SI_SYS:
> >>> err |= __put_user((compat_uptr_t)(unsigned long)
--
Catalin
On 2/13/2015 5:44 AM, Catalin Marinas wrote:
> On Fri, Feb 13, 2015 at 04:00:43PM +0800, Bamvor Jian Zhang wrote:
>> On 2015/2/11 23:40, Catalin Marinas wrote:
>>> On Wed, Feb 11, 2015 at 07:22:08PM +0800, Bamvor Jian Zhang wrote:
>>>> On 2015/2/10 20:27, Catalin Marinas wrote:
>>>>> On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
>> ...
>>> The native sigval_t is also a union but on 64-bit big endian, the
>>> sival_int overlaps with the most significant 32-bit of the sival_ptr.
>>> So reading sival_int would always be 0. When the compat siginfo is
>>> copied to user, arm64 reads the native sival_ptr (si_ptr) and converts
>>> it to the compat one, getting the correct 32-bit value. However, other
>>> architectures access sival_int (si_int) instead which breaks with your
>>> get_compat_sigevent() changes.
>
>> tile, s390: arch/xxx/kernel/compat_signal.c
>
> tile seems to be bi-endian, though I couldn't see a Kconfig option, nor
> something defining __BIG_ENDIAN__ in headers or Makefile. I guess it's
> coming from the compiler directly.
Yes, we just pick up the compiler's __BIG_ENDIAN__ if specified.
> Anyway, on big endian tile, I we have
> the same issue as on big endian arm64.
>
> I think it's only tile that needs fixing for big endian, something like
> the arm64 patch below:
>
>> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
>> index e299de396e9b..32601939a3c8 100644
>> --- a/arch/arm64/kernel/signal32.c
>> +++ b/arch/arm64/kernel/signal32.c
>> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>> case __SI_TIMER:
>> err |= __put_user(from->si_tid, &to->si_tid);
>> err |= __put_user(from->si_overrun, &to->si_overrun);
>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
>> - &to->si_ptr);
>> + err |= __put_user(from->si_int, &to->si_int);
>> break;
>> case __SI_POLL:
>> err |= __put_user(from->si_band, &to->si_band);
>> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>> case __SI_MESGQ: /* But this is */
>> err |= __put_user(from->si_pid, &to->si_pid);
>> err |= __put_user(from->si_uid, &to->si_uid);
>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
>> + err |= __put_user(from->si_int, &to->si_int);
>> break;
>> case __SI_SYS:
>> err |= __put_user((compat_uptr_t)(unsigned long)
I must be confused here, but I don't see that these do anything different.
If we are writing 32 bits to to->si_ptr or to->si_int, either way the high 32 bits
are irrelevant. So whether we read it from from->si_ptr and massage the high bits,
or just read it from from->si_int as a straight-up 32-bit quantity, either way it
seems we should end up writing the same bits to userspace.
I would understand the argument if we were overlaying the si_ptr/si_int union
from a kernel-side siginfo_t where si_ptr and si_int are different sizes
onto userspace, but it doesn't seem we ever do that.
All that said, it certainly seems like the si_int version is simpler, so I don't have
a problem with switching to it, but I don't see how it fixes a problem.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
On Fri, Feb 13, 2015 at 04:56:29PM -0500, Chris Metcalf wrote:
> On 2/13/2015 5:44 AM, Catalin Marinas wrote:
> >On Fri, Feb 13, 2015 at 04:00:43PM +0800, Bamvor Jian Zhang wrote:
> >>diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> >>index e299de396e9b..32601939a3c8 100644
> >>--- a/arch/arm64/kernel/signal32.c
> >>+++ b/arch/arm64/kernel/signal32.c
> >>@@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> >> case __SI_TIMER:
> >> err |= __put_user(from->si_tid, &to->si_tid);
> >> err |= __put_user(from->si_overrun, &to->si_overrun);
> >>- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
> >>- &to->si_ptr);
> >>+ err |= __put_user(from->si_int, &to->si_int);
> >> break;
> >> case __SI_POLL:
> >> err |= __put_user(from->si_band, &to->si_band);
> >>@@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> >> case __SI_MESGQ: /* But this is */
> >> err |= __put_user(from->si_pid, &to->si_pid);
> >> err |= __put_user(from->si_uid, &to->si_uid);
> >>- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
> >>+ err |= __put_user(from->si_int, &to->si_int);
> >> break;
> >> case __SI_SYS:
> >> err |= __put_user((compat_uptr_t)(unsigned long)
>
> I must be confused here, but I don't see that these do anything different.
>
> If we are writing 32 bits to to->si_ptr or to->si_int, either way the high 32 bits
> are irrelevant. So whether we read it from from->si_ptr and massage the high bits,
> or just read it from from->si_int as a straight-up 32-bit quantity, either way it
> seems we should end up writing the same bits to userspace.
>
> I would understand the argument if we were overlaying the si_ptr/si_int union
> from a kernel-side siginfo_t where si_ptr and si_int are different sizes
> onto userspace, but it doesn't seem we ever do that.
That's the problem, "from" above is a kernel siginfo_t while "to" is a
compat_siginfo_t. The call paths go something like:
1. user populates sival_int compat_sigevent and invokes
compat_sys_mq_notify()
2. kernel get_compat_sigevent() copies compat_sigevent into the native
sigevent. compat and native sival_int are the same, no problem so
far. The other half of 64-bit sival_ptr is zeroed by a memset in this
function (this other half can be top or bottom, depending on
endianness)
3. signal is about to be delivered to user via arch code. The
compat_ptr(from->si_ptr) conversion always takes the least
significant part of the native si_ptr. On big endian 64-bit, this is
zero because get_compat_sigevent() populated the top part of si_ptr
with si_int.
So delivering such signals to compat user always sets si_int to 0.
Little endian is fine.
A similar example is sys_timer_create() which takes a sigevent argument.
Maybe Bamvor has a test case.
(btw, I'm off for a week, I'll follow up when I get back)
--
Catalin
On 2015/2/14 19:22, Catalin Marinas wrote:
> On Fri, Feb 13, 2015 at 04:56:29PM -0500, Chris Metcalf wrote:
>> On 2/13/2015 5:44 AM, Catalin Marinas wrote:
>>> On Fri, Feb 13, 2015 at 04:00:43PM +0800, Bamvor Jian Zhang wrote:
>>>> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
>>>> index e299de396e9b..32601939a3c8 100644
>>>> --- a/arch/arm64/kernel/signal32.c
>>>> +++ b/arch/arm64/kernel/signal32.c
>>>> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>>> case __SI_TIMER:
>>>> err |= __put_user(from->si_tid, &to->si_tid);
>>>> err |= __put_user(from->si_overrun, &to->si_overrun);
>>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
>>>> - &to->si_ptr);
>>>> + err |= __put_user(from->si_int, &to->si_int);
>>>> break;
>>>> case __SI_POLL:
>>>> err |= __put_user(from->si_band, &to->si_band);
>>>> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>>> case __SI_MESGQ: /* But this is */
>>>> err |= __put_user(from->si_pid, &to->si_pid);
>>>> err |= __put_user(from->si_uid, &to->si_uid);
>>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
>>>> + err |= __put_user(from->si_int, &to->si_int);
>>>> break;
>>>> case __SI_SYS:
>>>> err |= __put_user((compat_uptr_t)(unsigned long)
>>
>> I must be confused here, but I don't see that these do anything different.
>>
>> If we are writing 32 bits to to->si_ptr or to->si_int, either way the high 32 bits
>> are irrelevant. So whether we read it from from->si_ptr and massage the high bits,
>> or just read it from from->si_int as a straight-up 32-bit quantity, either way it
>> seems we should end up writing the same bits to userspace.
>>
>> I would understand the argument if we were overlaying the si_ptr/si_int union
>> from a kernel-side siginfo_t where si_ptr and si_int are different sizes
>> onto userspace, but it doesn't seem we ever do that.
>
> That's the problem, "from" above is a kernel siginfo_t while "to" is a
> compat_siginfo_t. The call paths go something like:
>
> 1. user populates sival_int compat_sigevent and invokes
> compat_sys_mq_notify()
> 2. kernel get_compat_sigevent() copies compat_sigevent into the native
> sigevent. compat and native sival_int are the same, no problem so
> far. The other half of 64-bit sival_ptr is zeroed by a memset in this
> function (this other half can be top or bottom, depending on
> endianness)
> 3. signal is about to be delivered to user via arch code. The
> compat_ptr(from->si_ptr) conversion always takes the least
> significant part of the native si_ptr. On big endian 64-bit, this is
> zero because get_compat_sigevent() populated the top part of si_ptr
> with si_int.
>
> So delivering such signals to compat user always sets si_int to 0.
> Little endian is fine.
>
> A similar example is sys_timer_create() which takes a sigevent argument.
> Maybe Bamvor has a test case.
> A similar example is sys_timer_create() which takes a sigevent argument.
> Maybe Bamvor has a test case.
Yeap, this issue is came from glibc rt testcases(tst-cputimer1,
tst-cputimer2, tst-cputimer3, tst-timer4, tst-timer5). The above test
cases include timer_create syscall with sigevent.sigev_notify = SIGEV_THREAD.
They failed when they compiled as armeb elf and run on aarch64_be kernel.
When I try to fix it, I found sys_mq_notify is similar.
regards
bamvor
> (btw, I'm off for a week, I'll follow up when I get back)
>
On 2015/2/13 18:44, Catalin Marinas wrote:
> On Fri, Feb 13, 2015 at 04:00:43PM +0800, Bamvor Jian Zhang wrote:
>> On 2015/2/11 23:40, Catalin Marinas wrote:
>>> On Wed, Feb 11, 2015 at 07:22:08PM +0800, Bamvor Jian Zhang wrote:
>>>> On 2015/2/10 20:27, Catalin Marinas wrote:
>>>>> On Tue, Feb 10, 2015 at 10:10:11AM +0000, Zhang Jian(Bamvor) wrote:
>> ...
>>> The native sigval_t is also a union but on 64-bit big endian, the
>>> sival_int overlaps with the most significant 32-bit of the sival_ptr.
>>> So reading sival_int would always be 0. When the compat siginfo is
>>> copied to user, arm64 reads the native sival_ptr (si_ptr) and converts
>>> it to the compat one, getting the correct 32-bit value. However, other
>>> architectures access sival_int (si_int) instead which breaks with your
>>> get_compat_sigevent() changes.
>>>
>>>>> I think the correct fix is in the arm64 code:
>>>>
>>>> The following code could fix my issue.
>>>
>>> Without any parts of your patch?
>>
>> Yes. As you mentioned above, sival_int overlaps the most significant 32bit
>> of the sival_ptr, it seems that your patch is right if sival_ptr is less than
>> 32bit.
>
> I don't follow you here. sival_ptr is greater than 32-bit on a 64-bit
> kernel.
Sorry for confuse. I was considering if the pointer in sival_ptr is greater
than 32bit in 64bit application. But it seems that it is not relative to my
issue: We only talk about the 32bit application here.
>>> I think that's correct fix since in the SIGEV_THREAD mq_notify case, we
>>> would not deliver a signal as notification, so the sival_int value is
>>> irrelevant (it would be 0 for big-endian compat tasks because of the
>>> sigval_t union on 64-bit).
>>>
>>> Your patch would work as well but you have to change all the other
>>> architectures to use si_ptr when copying to a compat siginfo.
>>
>> Yeah, it seems that my patch is useful only if the sival_ptr is bigger
>> than 32bit. It need the similar changes with following catalin's patch
>> in the following 64bit architecture:
>>
>> x86: arch/x86/ia32/ia32_signal.c
>
> That's a little endian architecture and the use of ptr_to_compat() looks
> fine to me.
>
>> tile, s390: arch/xxx/kernel/compat_signal.c
>
> s390 uses si_int already, as in my proposed arm64 patch.
>
> tile seems to be bi-endian, though I couldn't see a Kconfig option, nor
> something defining __BIG_ENDIAN__ in headers or Makefile. I guess it's
> coming from the compiler directly. Anyway, on big endian tile, I we have
> the same issue as on big endian arm64.
>
>> parisc, sparc, mips: arch/xxx/kernel/signal32.c
>
> paris, sparc and mips all use si_int instead of si_ptr, so that's fine.
>
>> powerpc: arch/xxx/kernel/signal_32.c
>
> Same for powerpc, it uses si_int instead of si_ptr.
>
>> cc these maintainers for input.
>
> I think it's only tile that needs fixing for big endian, something like
> the arm64 patch below:
Agree. I was thinking if it is not very clear that app write to si_ptr in
userspace while kernel only read/write si_int.
regards
bamvor
>>>>> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
>>>>> index e299de396e9b..32601939a3c8 100644
>>>>> --- a/arch/arm64/kernel/signal32.c
>>>>> +++ b/arch/arm64/kernel/signal32.c
>>>>> @@ -154,8 +154,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>>>> case __SI_TIMER:
>>>>> err |= __put_user(from->si_tid, &to->si_tid);
>>>>> err |= __put_user(from->si_overrun, &to->si_overrun);
>>>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
>>>>> - &to->si_ptr);
>>>>> + err |= __put_user(from->si_int, &to->si_int);
>>>>> break;
>>>>> case __SI_POLL:
>>>>> err |= __put_user(from->si_band, &to->si_band);
>>>>> @@ -184,7 +183,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
>>>>> case __SI_MESGQ: /* But this is */
>>>>> err |= __put_user(from->si_pid, &to->si_pid);
>>>>> err |= __put_user(from->si_uid, &to->si_uid);
>>>>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, &to->si_ptr);
>>>>> + err |= __put_user(from->si_int, &to->si_int);
>>>>> break;
>>>>> case __SI_SYS:
>>>>> err |= __put_user((compat_uptr_t)(unsigned long)
>
On 2/14/2015 6:22 AM, Catalin Marinas wrote:
> 1. user populates sival_int compat_sigevent and invokes
> compat_sys_mq_notify()
> 2. kernel get_compat_sigevent() copies compat_sigevent into the native
> sigevent. compat and native sival_int are the same, no problem so
> far. The other half of 64-bit sival_ptr is zeroed by a memset in this
> function (this other half can be top or bottom, depending on
> endianness)
> 3. signal is about to be delivered to user via arch code. The
> compat_ptr(from->si_ptr) conversion always takes the least
> significant part of the native si_ptr. On big endian 64-bit, this is
> zero because get_compat_sigevent() populated the top part of si_ptr
> with si_int.
Thanks, that makes sense. I was missing the fact that the conversion
issue was actually around the in-kernel 64-bit version of the structure.
Using si_int consistently does seem like it should do the right thing;
I'll post a patch for tilegx32 big-endian to do the right thing here.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
On 2/14/2015 6:22 AM, Catalin Marinas wrote:
> 1. user populates sival_int compat_sigevent and invokes
> compat_sys_mq_notify()
> 2. kernel get_compat_sigevent() copies compat_sigevent into the native
> sigevent. compat and native sival_int are the same, no problem so
> far. The other half of 64-bit sival_ptr is zeroed by a memset in this
> function (this other half can be top or bottom, depending on
> endianness)
> 3. signal is about to be delivered to user via arch code. The
> compat_ptr(from->si_ptr) conversion always takes the least
> significant part of the native si_ptr. On big endian 64-bit, this is
> zero because get_compat_sigevent() populated the top part of si_ptr
> with si_int.
>
> So delivering such signals to compat user always sets si_int to 0.
> Little endian is fine.
I looked at this again as I was getting ready to do a tile patch, and realized
why tile and arm64 are different here: tile does a field-by-field copy in
copy_siginfo_from_user32(), like parisc and s390. As a result, we initialize
the 64-bit kernel si_ptr value by cast from the 32-bit user si_ptr value, rather
than blindly writing into the lower-addressed half of the 64-bit sigval.
As a result, I think I will leave the existing code alone, though unfortunately
that leaves it somewhat unique in manipulating the si_ptr field directly.
But I think the s390 and parisc copy_siginfo_from_user32 leave the high
bits of si_ptr uninitialized, which also strikes me as a bad idea in general.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
On Tue, Feb 24, 2015 at 04:54:17PM -0500, Chris Metcalf wrote:
> On 2/14/2015 6:22 AM, Catalin Marinas wrote:
> >1. user populates sival_int compat_sigevent and invokes
> > compat_sys_mq_notify()
> >2. kernel get_compat_sigevent() copies compat_sigevent into the native
> > sigevent. compat and native sival_int are the same, no problem so
> > far. The other half of 64-bit sival_ptr is zeroed by a memset in this
> > function (this other half can be top or bottom, depending on
> > endianness)
> >3. signal is about to be delivered to user via arch code. The
> > compat_ptr(from->si_ptr) conversion always takes the least
> > significant part of the native si_ptr. On big endian 64-bit, this is
> > zero because get_compat_sigevent() populated the top part of si_ptr
> > with si_int.
> >
> >So delivering such signals to compat user always sets si_int to 0.
> >Little endian is fine.
>
> I looked at this again as I was getting ready to do a tile patch, and realized
> why tile and arm64 are different here: tile does a field-by-field copy in
> copy_siginfo_from_user32(), like parisc and s390. As a result, we initialize
> the 64-bit kernel si_ptr value by cast from the 32-bit user si_ptr value, rather
> than blindly writing into the lower-addressed half of the 64-bit sigval.
It's not about copy_siginfo_from_user32() but the generic
get_compat_sigevent() which always uses sival_int (called from e.g.
compat_sys_timer_create()).
--
Catalin