[PATCH] compat: Fix endian issue in union sigval

[PATCH] compat: Fix endian issue in union sigval

[Zhang Jian(Bamvor)]

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) <bamvor.zhangjian@huawei.com>
---
 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

[PATCH] compat: Fix endian issue in union sigval

[Catalin Marinas]

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) <bamvor.zhangjian@huawei.com>
> ---
>  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

[PATCH] compat: Fix endian issue in union sigval

[Bamvor Jian Zhang]

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) <bamvor.zhangjian@huawei.com>
>> ---
>>  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
<viro@parcelfarce.linux.theplanet.co.uk> more than ten years ago[1]:

author   Alexander Viro <viro@parcelfarce.linux.theplanet.co.uk> 2004-07-13 04:02:57 (GMT)
committer   Linus Torvalds <torvalds@ppc970.osdl.org>   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.
> 

[PATCH] compat: Fix endian issue in union sigval

[Catalin Marinas]

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) <bamvor.zhangjian@huawei.com>
> >> ---
> >>  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

[PATCH] compat: Fix endian issue in union sigval

[Bamvor Jian Zhang]

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

[PATCH] compat: Fix endian issue in union sigval

[Catalin Marinas]

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

[PATCH] compat: Fix endian issue in union sigval

[Chris Metcalf]

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

[PATCH] compat: Fix endian issue in union sigval

[Catalin Marinas]

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

[PATCH] compat: Fix endian issue in union sigval

[Bamvor Jian Zhang]

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

[PATCH] compat: Fix endian issue in union sigval

[Bamvor Jian Zhang]

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

[PATCH] compat: Fix endian issue in union sigval

[Chris Metcalf]

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

[PATCH] compat: Fix endian issue in union sigval

[Chris Metcalf]

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

[PATCH] compat: Fix endian issue in union sigval

[Catalin Marinas]

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

[PATCH] tile: use si_int instead of si_ptr for compat_siginfo

[Chris Metcalf]

To be compatible with the generic get_compat_sigevent(), the
copy_siginfo_to_user32() and thus copy_siginfo_from_user32()
have to use si_int instead of si_ptr.  Using si_ptr means that
for the case of ILP32 compat code running in big-endian mode,
we would end up copying the high 32 bits of the pointer value
into si_int instead of the desired low 32 bits.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/tile/kernel/compat_signal.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c
index 8c5abf2e4794..bca13054afb4 100644
--- a/arch/tile/kernel/compat_signal.c
+++ b/arch/tile/kernel/compat_signal.c
@@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
 	if (from->si_code < 0) {
 		err |= __put_user(from->si_pid, &to->si_pid);
 		err |= __put_user(from->si_uid, &to->si_uid);
-		err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr);
+		err |= __put_user(from->si_int, &to->si_int);
 	} else {
 		/*
 		 * First 32bits of unions are always present:
@@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
 			break;
 		case __SI_TIMER >> 16:
 			err |= __put_user(from->si_overrun, &to->si_overrun);
-			err |= __put_user(ptr_to_compat(from->si_ptr),
-					  &to->si_ptr);
+			err |= __put_user(from->si_int, &to->si_int);
 			break;
 			 /* This is not generated by the kernel as of now.  */
 		case __SI_RT >> 16:
@@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
 int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
 {
 	int err;
-	u32 ptr32;
 
 	if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo)))
 		return -EFAULT;
@@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
 
 	err |= __get_user(to->si_pid, &from->si_pid);
 	err |= __get_user(to->si_uid, &from->si_uid);
-	err |= __get_user(ptr32, &from->si_ptr);
-	to->si_ptr = compat_ptr(ptr32);
+	err |= __get_user(to->si_int, &from->si_int);
 
 	return err;
 }
-- 
2.1.2

[PATCH] tile: use si_int instead of si_ptr for compat_siginfo

[Catalin Marinas]

On Mon, Mar 16, 2015 at 03:04:05PM -0400, Chris Metcalf wrote:
> To be compatible with the generic get_compat_sigevent(), the
> copy_siginfo_to_user32() and thus copy_siginfo_from_user32()
> have to use si_int instead of si_ptr.  Using si_ptr means that
> for the case of ILP32 compat code running in big-endian mode,
> we would end up copying the high 32 bits of the pointer value
> into si_int instead of the desired low 32 bits.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/tile/kernel/compat_signal.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c
> index 8c5abf2e4794..bca13054afb4 100644
> --- a/arch/tile/kernel/compat_signal.c
> +++ b/arch/tile/kernel/compat_signal.c
> @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>  	if (from->si_code < 0) {
>  		err |= __put_user(from->si_pid, &to->si_pid);
>  		err |= __put_user(from->si_uid, &to->si_uid);
> -		err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr);
> +		err |= __put_user(from->si_int, &to->si_int);
>  	} else {
>  		/*
>  		 * First 32bits of unions are always present:
> @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>  			break;
>  		case __SI_TIMER >> 16:
>  			err |= __put_user(from->si_overrun, &to->si_overrun);
> -			err |= __put_user(ptr_to_compat(from->si_ptr),
> -					  &to->si_ptr);
> +			err |= __put_user(from->si_int, &to->si_int);
>  			break;

It's usually the __SI_TIMER and __SI_MESGQ cases that matters here (the
latter already handled). I'm not entirely sure about the si_code < 0
change.

>  			 /* This is not generated by the kernel as of now.  */
>  		case __SI_RT >> 16:
> @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>  int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
>  {
>  	int err;
> -	u32 ptr32;
>  
>  	if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo)))
>  		return -EFAULT;
> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
>  
>  	err |= __get_user(to->si_pid, &from->si_pid);
>  	err |= __get_user(to->si_uid, &from->si_uid);
> -	err |= __get_user(ptr32, &from->si_ptr);
> -	to->si_ptr = compat_ptr(ptr32);
> +	err |= __get_user(to->si_int, &from->si_int);

We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I
can't see it on tile. Some members or even half of si_ptr would be left
uninitialised.

-- 
Catalin

[PATCH] tile: use si_int instead of si_ptr for compat_siginfo

[Chris Metcalf]

(+s390 and parisc maintainers)

On 03/23/2015 08:02 AM, Catalin Marinas wrote:
> On Mon, Mar 16, 2015 at 03:04:05PM -0400, Chris Metcalf wrote:
>> To be compatible with the generic get_compat_sigevent(), the
>> copy_siginfo_to_user32() and thus copy_siginfo_from_user32()
>> have to use si_int instead of si_ptr.  Using si_ptr means that
>> for the case of ILP32 compat code running in big-endian mode,
>> we would end up copying the high 32 bits of the pointer value
>> into si_int instead of the desired low 32 bits.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>>   arch/tile/kernel/compat_signal.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c
>> index 8c5abf2e4794..bca13054afb4 100644
>> --- a/arch/tile/kernel/compat_signal.c
>> +++ b/arch/tile/kernel/compat_signal.c
>> @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>>   	if (from->si_code < 0) {
>>   		err |= __put_user(from->si_pid, &to->si_pid);
>>   		err |= __put_user(from->si_uid, &to->si_uid);
>> -		err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr);
>> +		err |= __put_user(from->si_int, &to->si_int);
>>   	} else {
>>   		/*
>>   		 * First 32bits of unions are always present:
>> @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>>   			break;
>>   		case __SI_TIMER >> 16:
>>   			err |= __put_user(from->si_overrun, &to->si_overrun);
>> -			err |= __put_user(ptr_to_compat(from->si_ptr),
>> -					  &to->si_ptr);
>> +			err |= __put_user(from->si_int, &to->si_int);
>>   			break;
> It's usually the __SI_TIMER and __SI_MESGQ cases that matters here (the
> latter already handled). I'm not entirely sure about the si_code < 0
> change.

To be honest, I'm not even sure what path sets si_code < 0.  I see
that that is SI_FROMUSER(), but I don't see where it gets set.

In any case, I guess a risk here is that of a 64-bit process doing a 
sigqueue()
targeting a 32-bit process.  It seems like an impossible problem for the
32-bit process to know whether the 64-bit process wrote a 32-bit pointer
to the 64-bit sival_ptr field (and thus we should deliver the second 32-bit
word of the union sigval to the 32-bit user), or if the 64-bit process wrote
a 32-bit value to the 32-bit sival_int field (and thus we should deliver 
the
first 32-bit word of the union sigval).  Little-endian makes some things
a little bit easier :-)

All that said, my inclination is to use si_int here just because that's what
we're using elsewhere.  But I'm not entirely sure either.

>>   			 /* This is not generated by the kernel as of now.  */
>>   		case __SI_RT >> 16:
>> @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr
>>   int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
>>   {
>>   	int err;
>> -	u32 ptr32;
>>   
>>   	if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo)))
>>   		return -EFAULT;
>> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
>>   
>>   	err |= __get_user(to->si_pid, &from->si_pid);
>>   	err |= __get_user(to->si_uid, &from->si_uid);
>> -	err |= __get_user(ptr32, &from->si_ptr);
>> -	to->si_ptr = compat_ptr(ptr32);
>> +	err |= __get_user(to->si_int, &from->si_int);
> We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I
> can't see it on tile. Some members or even half of si_ptr would be left
> uninitialised.

So here we presumably have the reverse problem, which is a 32-bit
process doing a sigqueue() to a 64-bit process.  If the 64-bit process
inspects the sival_ptr, it does seem like it might find garbage in it.
But it also doesn't seem portable in much the same way as the
reverse direction; for a 32-bit process to signal a 64-bit process means
the 64-bit process can't read si_ptr or it will get different values
depending on what endianness is in force, so garbage is only part
of the problem.

I was modeling this code on the very similar code for parisc and s390.
I've added their maintainers to the cc list for this email thread.
I see that x86 uses si_ptr in its equivalent code, but of course it has no
issues with big-endianness.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

[PATCH] tile: use si_int instead of si_ptr for compat_siginfo

[Chris Metcalf]

On 03/23/2015 08:02 AM, Catalin Marinas wrote:
>> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from)
>> >  
>> >  	err |= __get_user(to->si_pid, &from->si_pid);
>> >  	err |= __get_user(to->si_uid, &from->si_uid);
>> >-	err |= __get_user(ptr32, &from->si_ptr);
>> >-	to->si_ptr = compat_ptr(ptr32);
>> >+	err |= __get_user(to->si_int, &from->si_int);
> We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I
> can't see it on tile. Some members or even half of si_ptr would be left
> uninitialised.

In the end I added a memset() for the tile compat case like you suggest 
for arm64.

Thanks!

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com