Re: [PATCH] compat: Fix endian issue in union sigval
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 +, 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 +, 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] compat: Fix endian issue in union sigval
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 +, 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) > >> --- > >> 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). You
Re: [PATCH] compat: Fix endian issue in union sigval
On 2015/2/10 20:27, Catalin Marinas wrote: > cc'ing linux-arch as well. > > On Tue, Feb 10, 2015 at 10:10:11AM +, 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) >> --- >> 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 more than ten years ago[1]: author Alexander Viro 2004-07-13 04:02:57 (GMT) committer Linus Torvalds2004-07-13 04:02:57 (GMT) commit 95b5842264ac470a1a3a59d2741bb18adb140c8b (patch) tree5167f68fae8f3bbc9b3a2d7617d1500356837c16 /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
Re: [PATCH] compat: Fix endian issue in union sigval
cc'ing linux-arch as well. On Tue, Feb 10, 2015 at 10:10:11AM +, 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) > --- > 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 -- To unsubs
[PATCH] compat: Fix endian issue in union sigval
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) --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/