| Message ID | 201503161908.t2GJ8fs5021877@farm-0002.internal.tilera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 16, 2015 at 03:04:05PM -0400, Chris Metcalf wrote: > To be compatible with the generic get_compat_sigevent(), the > copy_siginfo_to_user32() and thus copy_siginfo_from_user32() > have to use si_int instead of si_ptr. Using si_ptr means that > for the case of ILP32 compat code running in big-endian mode, > we would end up copying the high 32 bits of the pointer value > into si_int instead of the desired low 32 bits. > > Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > --- > arch/tile/kernel/compat_signal.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c > index 8c5abf2e4794..bca13054afb4 100644 > --- a/arch/tile/kernel/compat_signal.c > +++ b/arch/tile/kernel/compat_signal.c > @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr > if (from->si_code < 0) { > err |= __put_user(from->si_pid, &to->si_pid); > err |= __put_user(from->si_uid, &to->si_uid); > - err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr); > + err |= __put_user(from->si_int, &to->si_int); > } else { > /* > * First 32bits of unions are always present: > @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr > break; > case __SI_TIMER >> 16: > err |= __put_user(from->si_overrun, &to->si_overrun); > - err |= __put_user(ptr_to_compat(from->si_ptr), > - &to->si_ptr); > + err |= __put_user(from->si_int, &to->si_int); > break; It's usually the __SI_TIMER and __SI_MESGQ cases that matters here (the latter already handled). I'm not entirely sure about the si_code < 0 change. > /* This is not generated by the kernel as of now. */ > case __SI_RT >> 16: > @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr > int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) > { > int err; > - u32 ptr32; > > if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo))) > return -EFAULT; > @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) > > err |= __get_user(to->si_pid, &from->si_pid); > err |= __get_user(to->si_uid, &from->si_uid); > - err |= __get_user(ptr32, &from->si_ptr); > - to->si_ptr = compat_ptr(ptr32); > + err |= __get_user(to->si_int, &from->si_int); We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I can't see it on tile. Some members or even half of si_ptr would be left uninitialised.
(+s390 and parisc maintainers) On 03/23/2015 08:02 AM, Catalin Marinas wrote: > On Mon, Mar 16, 2015 at 03:04:05PM -0400, Chris Metcalf wrote: >> To be compatible with the generic get_compat_sigevent(), the >> copy_siginfo_to_user32() and thus copy_siginfo_from_user32() >> have to use si_int instead of si_ptr. Using si_ptr means that >> for the case of ILP32 compat code running in big-endian mode, >> we would end up copying the high 32 bits of the pointer value >> into si_int instead of the desired low 32 bits. >> >> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> --- >> arch/tile/kernel/compat_signal.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c >> index 8c5abf2e4794..bca13054afb4 100644 >> --- a/arch/tile/kernel/compat_signal.c >> +++ b/arch/tile/kernel/compat_signal.c >> @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> if (from->si_code < 0) { >> err |= __put_user(from->si_pid, &to->si_pid); >> err |= __put_user(from->si_uid, &to->si_uid); >> - err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr); >> + err |= __put_user(from->si_int, &to->si_int); >> } else { >> /* >> * First 32bits of unions are always present: >> @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> break; >> case __SI_TIMER >> 16: >> err |= __put_user(from->si_overrun, &to->si_overrun); >> - err |= __put_user(ptr_to_compat(from->si_ptr), >> - &to->si_ptr); >> + err |= __put_user(from->si_int, &to->si_int); >> break; > It's usually the __SI_TIMER and __SI_MESGQ cases that matters here (the > latter already handled). I'm not entirely sure about the si_code < 0 > change. To be honest, I'm not even sure what path sets si_code < 0. I see that that is SI_FROMUSER(), but I don't see where it gets set. In any case, I guess a risk here is that of a 64-bit process doing a sigqueue() targeting a 32-bit process. It seems like an impossible problem for the 32-bit process to know whether the 64-bit process wrote a 32-bit pointer to the 64-bit sival_ptr field (and thus we should deliver the second 32-bit word of the union sigval to the 32-bit user), or if the 64-bit process wrote a 32-bit value to the 32-bit sival_int field (and thus we should deliver the first 32-bit word of the union sigval). Little-endian makes some things a little bit easier :-) All that said, my inclination is to use si_int here just because that's what we're using elsewhere. But I'm not entirely sure either. >> /* This is not generated by the kernel as of now. */ >> case __SI_RT >> 16: >> @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) >> { >> int err; >> - u32 ptr32; >> >> if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo))) >> return -EFAULT; >> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) >> >> err |= __get_user(to->si_pid, &from->si_pid); >> err |= __get_user(to->si_uid, &from->si_uid); >> - err |= __get_user(ptr32, &from->si_ptr); >> - to->si_ptr = compat_ptr(ptr32); >> + err |= __get_user(to->si_int, &from->si_int); > We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I > can't see it on tile. Some members or even half of si_ptr would be left > uninitialised. So here we presumably have the reverse problem, which is a 32-bit process doing a sigqueue() to a 64-bit process. If the 64-bit process inspects the sival_ptr, it does seem like it might find garbage in it. But it also doesn't seem portable in much the same way as the reverse direction; for a 32-bit process to signal a 64-bit process means the 64-bit process can't read si_ptr or it will get different values depending on what endianness is in force, so garbage is only part of the problem. I was modeling this code on the very similar code for parisc and s390. I've added their maintainers to the cc list for this email thread. I see that x86 uses si_ptr in its equivalent code, but of course it has no issues with big-endianness.
On 03/23/2015 08:02 AM, Catalin Marinas wrote: >> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) >> > >> > err |= __get_user(to->si_pid, &from->si_pid); >> > err |= __get_user(to->si_uid, &from->si_uid); >> >- err |= __get_user(ptr32, &from->si_ptr); >> >- to->si_ptr = compat_ptr(ptr32); >> >+ err |= __get_user(to->si_int, &from->si_int); > We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I > can't see it on tile. Some members or even half of si_ptr would be left > uninitialised. In the end I added a memset() for the tile compat case like you suggest for arm64. Thanks!
diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c index 8c5abf2e4794..bca13054afb4 100644 --- a/arch/tile/kernel/compat_signal.c +++ b/arch/tile/kernel/compat_signal.c @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr if (from->si_code < 0) { err |= __put_user(from->si_pid, &to->si_pid); err |= __put_user(from->si_uid, &to->si_uid); - err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr); + err |= __put_user(from->si_int, &to->si_int); } else { /* * First 32bits of unions are always present: @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr break; case __SI_TIMER >> 16: err |= __put_user(from->si_overrun, &to->si_overrun); - err |= __put_user(ptr_to_compat(from->si_ptr), - &to->si_ptr); + err |= __put_user(from->si_int, &to->si_int); break; /* This is not generated by the kernel as of now. */ case __SI_RT >> 16: @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) { int err; - u32 ptr32; if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo))) return -EFAULT; @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) err |= __get_user(to->si_pid, &from->si_pid); err |= __get_user(to->si_uid, &from->si_uid); - err |= __get_user(ptr32, &from->si_ptr); - to->si_ptr = compat_ptr(ptr32); + err |= __get_user(to->si_int, &from->si_int); return err; }
To be compatible with the generic get_compat_sigevent(), the copy_siginfo_to_user32() and thus copy_siginfo_from_user32() have to use si_int instead of si_ptr. Using si_ptr means that for the case of ILP32 compat code running in big-endian mode, we would end up copying the high 32 bits of the pointer value into si_int instead of the desired low 32 bits. Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com> Cc: Catalin Marinas <catalin.marinas@arm.com> --- arch/tile/kernel/compat_signal.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)