Use 64-bit longs instead of 32-bit pairs for time in compat_shmid64_ds when in compat64. Update copy_compat_shmid_to_user to support the compat64 struct.
Don't limit shmmax to INT_MAX when in compat64.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- arch/arm64/include/asm/compat.h | 6 ++++++ ipc/shm.c | 8 ++++++++ 2 files changed, 14 insertions(+)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 962f954fd194..fd5d27ea2ef5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -166,12 +166,18 @@ struct compat_msqid64_ds { struct compat_shmid64_ds { struct compat_ipc64_perm shm_perm; compat_size_t shm_segsz; +#ifdef CONFIG_COMPAT64 + compat_ulong_t shm_atime; + compat_ulong_t shm_dtime; + compat_ulong_t shm_ctime; +#else compat_ulong_t shm_atime; compat_ulong_t shm_atime_high; compat_ulong_t shm_dtime; compat_ulong_t shm_dtime_high; compat_ulong_t shm_ctime; compat_ulong_t shm_ctime_high; +#endif compat_pid_t shm_cpid; compat_pid_t shm_lpid; compat_ulong_t shm_nattch; diff --git a/ipc/shm.c b/ipc/shm.c index 6d929205bc41..458d42568fa9 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1336,8 +1336,10 @@ struct compat_shm_info { static int copy_compat_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version) { +#ifndef CONFIG_COMPAT64 if (in->shmmax > INT_MAX) in->shmmax = INT_MAX; +#endif if (version == IPC_64) { struct compat_shminfo64 info; memset(&info, 0, sizeof(info)); @@ -1381,12 +1383,18 @@ static int copy_compat_shmid_to_user(void __user *buf, struct shmid64_ds *in, struct compat_shmid64_ds v; memset(&v, 0, sizeof(v)); to_compat_ipc64_perm(&v.shm_perm, &in->shm_perm); +#ifdef CONFIG_COMPAT64 + v.shm_atime = in->shm_atime; + v.shm_dtime = in->shm_dtime; + v.shm_ctime = in->shm_ctime; +#else v.shm_atime = lower_32_bits(in->shm_atime); v.shm_atime_high = upper_32_bits(in->shm_atime); v.shm_dtime = lower_32_bits(in->shm_dtime); v.shm_dtime_high = upper_32_bits(in->shm_dtime); v.shm_ctime = lower_32_bits(in->shm_ctime); v.shm_ctime_high = upper_32_bits(in->shm_ctime); +#endif v.shm_segsz = in->shm_segsz; v.shm_nattch = in->shm_nattch; v.shm_cpid = in->shm_cpid;
Hi Teo,
On 23-09-2022 13:03, Teo Couprie Diaz wrote:
Use 64-bit longs instead of 32-bit pairs for time in compat_shmid64_ds when in compat64. Update copy_compat_shmid_to_user to support the compat64 struct.
Don't limit shmmax to INT_MAX when in compat64.
LGTM!
Thanks, Tudor
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
arch/arm64/include/asm/compat.h | 6 ++++++ ipc/shm.c | 8 ++++++++ 2 files changed, 14 insertions(+)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 962f954fd194..fd5d27ea2ef5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -166,12 +166,18 @@ struct compat_msqid64_ds { struct compat_shmid64_ds { struct compat_ipc64_perm shm_perm; compat_size_t shm_segsz; +#ifdef CONFIG_COMPAT64
- compat_ulong_t shm_atime;
- compat_ulong_t shm_dtime;
- compat_ulong_t shm_ctime;
+#else compat_ulong_t shm_atime; compat_ulong_t shm_atime_high; compat_ulong_t shm_dtime; compat_ulong_t shm_dtime_high; compat_ulong_t shm_ctime; compat_ulong_t shm_ctime_high; +#endif compat_pid_t shm_cpid; compat_pid_t shm_lpid; compat_ulong_t shm_nattch; diff --git a/ipc/shm.c b/ipc/shm.c index 6d929205bc41..458d42568fa9 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1336,8 +1336,10 @@ struct compat_shm_info { static int copy_compat_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version) { +#ifndef CONFIG_COMPAT64 if (in->shmmax > INT_MAX) in->shmmax = INT_MAX; +#endif if (version == IPC_64) { struct compat_shminfo64 info; memset(&info, 0, sizeof(info)); @@ -1381,12 +1383,18 @@ static int copy_compat_shmid_to_user(void __user *buf, struct shmid64_ds *in, struct compat_shmid64_ds v; memset(&v, 0, sizeof(v)); to_compat_ipc64_perm(&v.shm_perm, &in->shm_perm); +#ifdef CONFIG_COMPAT64
v.shm_atime = in->shm_atime;v.shm_dtime = in->shm_dtime;v.shm_ctime = in->shm_ctime;+#else v.shm_atime = lower_32_bits(in->shm_atime); v.shm_atime_high = upper_32_bits(in->shm_atime); v.shm_dtime = lower_32_bits(in->shm_dtime); v.shm_dtime_high = upper_32_bits(in->shm_dtime); v.shm_ctime = lower_32_bits(in->shm_ctime); v.shm_ctime_high = upper_32_bits(in->shm_ctime); +#endif v.shm_segsz = in->shm_segsz; v.shm_nattch = in->shm_nattch; v.shm_cpid = in->shm_cpid;
On 23/09/2022 14:03, Teo Couprie Diaz wrote:
Use 64-bit longs instead of 32-bit pairs for time in compat_shmid64_ds when in compat64. Update copy_compat_shmid_to_user to support the compat64 struct.
Don't limit shmmax to INT_MAX when in compat64.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
arch/arm64/include/asm/compat.h | 6 ++++++
As a general rule it is undesirable to change both arch-specific and generic code in the same patch. Sometimes it is hard to avoid though, and it is slightly borderline here. I think it should be possible to have the generic patch first and then the arm64 one after (without breaking anything that is not already broken).
In that case, in terms of commit tags, the generic one could be "ipc/shm:" and the arm64 one "arm64: compat:".
ipc/shm.c | 8 ++++++++ 2 files changed, 14 insertions(+)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 962f954fd194..fd5d27ea2ef5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h
Trying to compare each compat struct with the corresponding native one, don't we have a problem with compat_ipc64_perm? __kernel_mode_t is unsigned int by default (including on arm64), while compat_ipc64_perm defines mode as unsigned short. I think we are getting away with it because of the padding, resulting in the struct having the same layout overall, and little endianness results in things working as long as you don't use a bigger value than 2^16. Still, worth fixing.
@@ -166,12 +166,18 @@ struct compat_msqid64_ds { struct compat_shmid64_ds { struct compat_ipc64_perm shm_perm; compat_size_t shm_segsz; +#ifdef CONFIG_COMPAT64
- compat_ulong_t shm_atime;
- compat_ulong_t shm_dtime;
- compat_ulong_t shm_ctime;
More precisely these should be compat_long_t to match the native struct.
+#else compat_ulong_t shm_atime; compat_ulong_t shm_atime_high; compat_ulong_t shm_dtime; compat_ulong_t shm_dtime_high; compat_ulong_t shm_ctime; compat_ulong_t shm_ctime_high; +#endif compat_pid_t shm_cpid; compat_pid_t shm_lpid; compat_ulong_t shm_nattch; diff --git a/ipc/shm.c b/ipc/shm.c index 6d929205bc41..458d42568fa9 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1336,8 +1336,10 @@ struct compat_shm_info { static int copy_compat_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version) { +#ifndef CONFIG_COMPAT64
You can use #ifdef CONFIG_COMPAT32 to avoid the negation.
if (in->shmmax > INT_MAX) in->shmmax = INT_MAX; +#endif if (version == IPC_64) { struct compat_shminfo64 info; memset(&info, 0, sizeof(info)); @@ -1381,12 +1383,18 @@ static int copy_compat_shmid_to_user(void __user *buf, struct shmid64_ds *in, struct compat_shmid64_ds v; memset(&v, 0, sizeof(v)); to_compat_ipc64_perm(&v.shm_perm, &in->shm_perm); +#ifdef CONFIG_COMPAT64
v.shm_atime = in->shm_atime;v.shm_dtime = in->shm_dtime;v.shm_ctime = in->shm_ctime;+#else v.shm_atime = lower_32_bits(in->shm_atime); v.shm_atime_high = upper_32_bits(in->shm_atime); v.shm_dtime = lower_32_bits(in->shm_dtime); v.shm_dtime_high = upper_32_bits(in->shm_dtime); v.shm_ctime = lower_32_bits(in->shm_ctime); v.shm_ctime_high = upper_32_bits(in->shm_ctime); +#endif v.shm_segsz = in->shm_segsz; v.shm_nattch = in->shm_nattch; v.shm_cpid = in->shm_cpid;
Nice patch, I'm just asking for a few more things as I realise there is more brokenness than I thought!
While we're at it, let's make ARCH_WANT_COMPAT_IPC_PARSE_VERSION depend on COMPAT32 instead of COMPAT in arch/arm64/Kconfig. I was trying to wrap my head around the old_shmctl / versioning complexity and figured out there's no need as this is not used in 64-bit.
Kevin
On 9/26/22 14:58, Kevin Brodsky wrote:
On 23/09/2022 14:03, Teo Couprie Diaz wrote:
Use 64-bit longs instead of 32-bit pairs for time in compat_shmid64_ds when in compat64. Update copy_compat_shmid_to_user to support the compat64 struct.
Don't limit shmmax to INT_MAX when in compat64.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
arch/arm64/include/asm/compat.h | 6 ++++++
As a general rule it is undesirable to change both arch-specific and generic code in the same patch. Sometimes it is hard to avoid though, and it is slightly borderline here. I think it should be possible to have the generic patch first and then the arm64 one after (without breaking anything that is not already broken).
In that case, in terms of commit tags, the generic one could be "ipc/shm:" and the arm64 one "arm64: compat:".
It did feel weird to have both of them in the same commit, so that makes sense. Split in v2.
ipc/shm.c | 8 ++++++++ 2 files changed, 14 insertions(+)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 962f954fd194..fd5d27ea2ef5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h
Trying to compare each compat struct with the corresponding native one, don't we have a problem with compat_ipc64_perm? __kernel_mode_t is unsigned int by default (including on arm64), while compat_ipc64_perm defines mode as unsigned short. I think we are getting away with it because of the padding, resulting in the struct having the same layout overall, and little endianness results in things working as long as you don't use a bigger value than 2^16. Still, worth fixing.
You are correct : changed to unsigned int and removed padding in compat64.
@@ -166,12 +166,18 @@ struct compat_msqid64_ds { struct compat_shmid64_ds { struct compat_ipc64_perm shm_perm; compat_size_t shm_segsz; +#ifdef CONFIG_COMPAT64
- compat_ulong_t shm_atime;
- compat_ulong_t shm_dtime;
- compat_ulong_t shm_ctime;
More precisely these should be compat_long_t to match the native struct.
I did miss that, thanks !
+#else compat_ulong_t shm_atime; compat_ulong_t shm_atime_high; compat_ulong_t shm_dtime; compat_ulong_t shm_dtime_high; compat_ulong_t shm_ctime; compat_ulong_t shm_ctime_high; +#endif compat_pid_t shm_cpid; compat_pid_t shm_lpid; compat_ulong_t shm_nattch; diff --git a/ipc/shm.c b/ipc/shm.c index 6d929205bc41..458d42568fa9 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1336,8 +1336,10 @@ struct compat_shm_info { static int copy_compat_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version) { +#ifndef CONFIG_COMPAT64
You can use #ifdef CONFIG_COMPAT32 to avoid the negation.
Fixed.
if (in->shmmax > INT_MAX) in->shmmax = INT_MAX;+#endif if (version == IPC_64) { struct compat_shminfo64 info; memset(&info, 0, sizeof(info)); @@ -1381,12 +1383,18 @@ static int copy_compat_shmid_to_user(void __user *buf, struct shmid64_ds *in, struct compat_shmid64_ds v; memset(&v, 0, sizeof(v)); to_compat_ipc64_perm(&v.shm_perm, &in->shm_perm); +#ifdef CONFIG_COMPAT64
v.shm_atime = in->shm_atime;v.shm_dtime = in->shm_dtime;v.shm_ctime = in->shm_ctime;+#else v.shm_atime = lower_32_bits(in->shm_atime); v.shm_atime_high = upper_32_bits(in->shm_atime); v.shm_dtime = lower_32_bits(in->shm_dtime); v.shm_dtime_high = upper_32_bits(in->shm_dtime); v.shm_ctime = lower_32_bits(in->shm_ctime); v.shm_ctime_high = upper_32_bits(in->shm_ctime); +#endif v.shm_segsz = in->shm_segsz; v.shm_nattch = in->shm_nattch; v.shm_cpid = in->shm_cpid;
Nice patch, I'm just asking for a few more things as I realise there is more brokenness than I thought!
No worries, thanks for the review !
While we're at it, let's make ARCH_WANT_COMPAT_IPC_PARSE_VERSION depend on COMPAT32 instead of COMPAT in arch/arm64/Kconfig. I was trying to wrap my head around the old_shmctl / versioning complexity and figured out there's no need as this is not used in 64-bit.
That makes sense, added on top of the other patches in v2.
Kevin
Cheers, Téo IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Teo Couprie Diaz -
Tudor Cretu