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 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. 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 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, >si_tid); >err |= __put_user(from->si_overrun, >si_overrun); > - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > ->si_ptr); > + err |= __put_user(from->si_int, >si_int); > break; > case __SI_POLL: > err |= __put_user(from->si_band, >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, >si_pid); > err |= __put_user(from->si_uid, >si_uid); > - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > >si_ptr); > + err |= __put_user(from->si_int, >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, >si_tid); err |= __put_user(from->si_overrun, >si_overrun); - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, - >si_ptr); + err |= __put_user(from->si_int, >si_int); break; case __SI_POLL: err |= __put_user(from->si_band, >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, >si_pid); err |= __put_user(from->si_uid, >si_uid); - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, >si_ptr); + err |= __put_user(from->si_int, >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 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 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 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, >si_tid); > >> err |= __put_user(from->si_overrun, >si_overrun); > >>-err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > >>- >si_ptr); > >>+err |= __put_user(from->si_int, >si_int); > >>break; > >>case __SI_POLL: > >>err |= __put_user(from->si_band, >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, >si_pid); > >>err |= __put_user(from->si_uid, >si_uid); > >>- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > >>>si_ptr); > >>+ err |= __put_user(from->si_int, >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 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, >si_tid); err |= __put_user(from->si_overrun, >si_overrun); -err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, - >si_ptr); +err |= __put_user(from->si_int, >si_int); break; case __SI_POLL: err |= __put_user(from->si_band, >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, >si_pid); err |= __put_user(from->si_uid, >si_uid); - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, >si_ptr); + err |= __put_user(from->si_int, >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, >si_tid); > >>>err |= __put_user(from->si_overrun, >si_overrun); > >>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > >>> ->si_ptr); > >>> + err |= __put_user(from->si_int, >si_int); > >>> break; > >>> case __SI_POLL: > >>> err |= __put_user(from->si_band, >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, >si_pid); > >>> err |= __put_user(from->si_uid, >si_uid); > >>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, > >>> >si_ptr); > >>> + err |= __put_user(from->si_int, >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, >si_tid); >>> err |= __put_user(from->si_overrun, >si_overrun); >>> -err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, >>> - >si_ptr); >>> +err |= __put_user(from->si_int, >si_int); >>> break; >>> case __SI_POLL: >>> err |= __put_user(from->si_band, >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, >si_pid); >>> err |= __put_user(from->si_uid, >si_uid); >>> - err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr, >>> >si_ptr); >>> + err |= __put_user(from->si_int, >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 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 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 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(, 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, , sizeof(*p)))
> >> + if (get_compat_sigevent(, u_notification) ||
> >> + copy_to_user(p, , 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,
> >> - _event->sigev_value.sival_int) ||
> >> + __get_user(*(long long*)event->sigev_value.sival_ptr,
>
> should be:
> __get_user(event->sigev_value.sival_ptr,
>
> > > + _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
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(, 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, , sizeof(*p)))
>> +if (get_compat_sigevent(, u_notification) ||
>> +copy_to_user(p, , 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,
>> -_event->sigev_value.sival_int) ||
>> +__get_user(*(long long*)event->sigev_value.sival_ptr,
should be:
__get_user(event->sigev_value.sival_ptr,
> > + _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
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) bamvor.zhangj...@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
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) bamvor.zhangj...@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
v...@parcelfarce.linux.theplanet.co.uk more than ten years ago[1]:
author Alexander Viro v...@parcelfarce.linux.theplanet.co.uk 2004-07-13
04:02:57 (GMT)
committer Linus Torvalds torva...@ppc970.osdl.org 2004-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/signal32.c
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(, 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, , sizeof(*p)))
> + if (get_compat_sigevent(, u_notification) ||
> + copy_to_user(p, , 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,
> - _event->sigev_value.sival_int) ||
> + __get_user(*(long long*)event->sigev_value.sival_ptr,
> + _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, >si_tid);
err |= __put_user(from->si_overrun, >si_overrun);
-err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
- >si_ptr);
+err |= __put_user(from->si_int, >si_int);
break;
case __SI_POLL:
err |= __put_user(from->si_band, >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, >si_pid);
err |= __put_user(from->si_uid, >si_uid);
- err |= __put_user((compat_uptr_t)(unsigned long)from->si_ptr,
>si_ptr);
+ err |= __put_user(from->si_int, >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 unsubscribe from this list: send the line
[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(, 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, , sizeof(*p)))
+ if (get_compat_sigevent(, u_notification) ||
+ copy_to_user(p, , 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,
- _event->sigev_value.sival_int) ||
+ __get_user(*(long long*)event->sigev_value.sival_ptr,
+ _event->sigev_value.sival_ptr) ||
__get_user(event->sigev_signo, _event->sigev_signo) ||
__get_user(event->sigev_notify, _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/
[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) bamvor.zhangj...@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
--
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
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) bamvor.zhangj...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of
