hpet_compat_ioctl and read file operations failed to handle parameters from
32-bit userspace and thus samples/timers/hpet_example.c fails as below.

root@intel-x86-64:~# ./hpet_example-32.out poll /dev/hpet 1 2
-hpet: executing poll
hpet_poll: HPET_IRQFREQ failed

This patch fixes cmd and arg handling in hpet_compat_ioctl and adds compat
handling for 32-bit userspace in hpet_read.

hpet_example now shows that it works for both 64-bit and 32-bit.

root@intel-x86-64:~# ./hpet_example-32.out poll /dev/hpet 1 2
-hpet: executing poll
hpet_poll: info.hi_flags 0x0
hpet_poll: expired time = 0xf4298
hpet_poll: revents = 0x1
hpet_poll: data 0x1
hpet_poll: expired time = 0xf4235
hpet_poll: revents = 0x1
hpet_poll: data 0x1
root@intel-x86-64:~# ./hpet_example-64.out poll /dev/hpet 1 2
-hpet: executing poll
hpet_poll: info.hi_flags 0x0
hpet_poll: expired time = 0xf42a1
hpet_poll: revents = 0x1
hpet_poll: data 0x1
hpet_poll: expired time = 0xf4232
hpet_poll: revents = 0x1
hpet_poll: data 0x1

Cc: stable@vger.kernel.org
Signed-off-by: He Zhe <zhe.he@windriver.com>
---
v2:
- Use in_compat_syscall to determine if we're handling 32-bit or 64-bit
- Drop unnecessary compat_ptr for hpet_ioctl_common
- Add comment for COMPAT_HPET_INFO and COMPAT_HPET_IRQFREQ

 drivers/char/hpet.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index d51fc8321d41..4eef4e926d4a 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -269,8 +269,18 @@ hpet_read(struct file *file, char __user *buf, size_t count, loff_t * ppos)
 	if (!devp->hd_ireqfreq)
 		return -EIO;
 
+#ifdef CONFIG_COMPAT
+	if (in_compat_syscall()) {
+		if (count < sizeof(compat_ulong_t))
+			return -EINVAL;
+	} else {
+		if (count < sizeof(unsigned long))
+			return -EINVAL;
+	}
+#else
 	if (count < sizeof(unsigned long))
 		return -EINVAL;
+#endif
 
 	add_wait_queue(&devp->hd_waitqueue, &wait);
 
@@ -294,9 +304,22 @@ hpet_read(struct file *file, char __user *buf, size_t count, loff_t * ppos)
 		schedule();
 	}
 
+#ifdef CONFIG_COMPAT
+	if (in_compat_syscall()) {
+		retval = put_user(data, (compat_ulong_t __user *)buf);
+		if (!retval)
+			retval = sizeof(compat_ulong_t);
+	} else {
+		retval = put_user(data, (unsigned long __user *)buf);
+		if (!retval)
+			retval = sizeof(unsigned long);
+	}
+#else
 	retval = put_user(data, (unsigned long __user *)buf);
 	if (!retval)
 		retval = sizeof(unsigned long);
+#endif
+
 out:
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&devp->hd_waitqueue, &wait);
@@ -651,12 +674,24 @@ struct compat_hpet_info {
 	unsigned short hi_timer;
 };
 
+/* 32-bit types would lead to different command codes which should be
+ * translated into 64-bit ones before passed to hpet_ioctl_common
+ */
+#define COMPAT_HPET_INFO       _IOR('h', 0x03, struct compat_hpet_info)
+#define COMPAT_HPET_IRQFREQ    _IOW('h', 0x6, compat_ulong_t)
+
 static long
 hpet_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct hpet_info info;
 	int err;
 
+	if (cmd == COMPAT_HPET_INFO)
+		cmd = HPET_INFO;
+
+	if (cmd == COMPAT_HPET_IRQFREQ)
+		cmd = HPET_IRQFREQ;
+
 	mutex_lock(&hpet_mutex);
 	err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
 	mutex_unlock(&hpet_mutex);
-- 
2.25.1

On Thu, Jun 6, 2024, at 11:46, He Zhe wrote:
> v2:
> - Use in_compat_syscall to determine if we're handling 32-bit or 64-bit
> - Drop unnecessary compat_ptr for hpet_ioctl_common
> - Add comment for COMPAT_HPET_INFO and COMPAT_HPET_IRQFREQ

Thanks, this version looks correct to me,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

I would suggest one simplification though:

> +#ifdef CONFIG_COMPAT
> +	if (in_compat_syscall()) {
> +		if (count < sizeof(compat_ulong_t))
> +			return -EINVAL;
> +	} else {
> +		if (count < sizeof(unsigned long))
> +			return -EINVAL;
> +	}
> +#else
>  	if (count < sizeof(unsigned long))
>  		return -EINVAL;
> +#endif

The #ifdef/#else is not really required here, since
in_compat_syscall() is defined to return false when
this is unset. For both cases, it should be sufficient
to keep the part inside of the #ifdef block.

     Arnd

On 2024/6/6 18:34, Arnd Bergmann wrote:
> On Thu, Jun 6, 2024, at 11:46, He Zhe wrote:
>> v2:
>> - Use in_compat_syscall to determine if we're handling 32-bit or 64-bit
>> - Drop unnecessary compat_ptr for hpet_ioctl_common
>> - Add comment for COMPAT_HPET_INFO and COMPAT_HPET_IRQFREQ
> Thanks, this version looks correct to me,
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
> I would suggest one simplification though:

Thanks. v3 is sent.

Zhe

>
>> +#ifdef CONFIG_COMPAT
>> +	if (in_compat_syscall()) {
>> +		if (count < sizeof(compat_ulong_t))
>> +			return -EINVAL;
>> +	} else {
>> +		if (count < sizeof(unsigned long))
>> +			return -EINVAL;
>> +	}
>> +#else
>>  	if (count < sizeof(unsigned long))
>>  		return -EINVAL;
>> +#endif
> The #ifdef/#else is not really required here, since
> in_compat_syscall() is defined to return false when
> this is unset. For both cases, it should be sufficient
> to keep the part inside of the #ifdef block.
>
>      Arnd