[PATCH] hpet: Support 32-bit userspace

[PATCH] hpet: Support 32-bit userspace

[He Zhe]

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, based on the
method in commit 3418ff76119d
("[PATCH] RTC: rtc-dev tweak for 64-bit kernel").

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>
---
 drivers/char/hpet.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index d51fc8321d41..025a5905a370 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -269,7 +269,8 @@ hpet_read(struct file *file, char __user *buf, size_t count, loff_t * ppos)
 	if (!devp->hd_ireqfreq)
 		return -EIO;
 
-	if (count < sizeof(unsigned long))
+	if ((sizeof(int) != sizeof(long) && count < sizeof(compat_ulong_t)) ||
+	    (sizeof(int) == sizeof(long) && count < sizeof(unsigned long)))
 		return -EINVAL;
 
 	add_wait_queue(&devp->hd_waitqueue, &wait);
@@ -294,9 +295,15 @@ hpet_read(struct file *file, char __user *buf, size_t count, loff_t * ppos)
 		schedule();
 	}
 
-	retval = put_user(data, (unsigned long __user *)buf);
-	if (!retval)
-		retval = sizeof(unsigned long);
+	if (sizeof(int) != sizeof(long) && count == sizeof(compat_ulong_t)) {
+		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);
+	}
 out:
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&devp->hd_waitqueue, &wait);
@@ -651,14 +658,23 @@ struct compat_hpet_info {
 	unsigned short hi_timer;
 };
 
+#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);
+	err = hpet_ioctl_common(file->private_data, cmd, (unsigned long)compat_ptr(arg), &info);
 	mutex_unlock(&hpet_mutex);
 
 	if ((cmd == HPET_INFO) && !err) {
-- 
2.25.1

Re: [PATCH] hpet: Support 32-bit userspace

[Arnd Bergmann]

On Thu, Jun 6, 2024, at 08:13, He Zhe wrote:
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index d51fc8321d41..025a5905a370 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -269,7 +269,8 @@ hpet_read(struct file *file, char __user *buf, 
> size_t count, loff_t * ppos)
>  	if (!devp->hd_ireqfreq)
>  		return -EIO;
> 
> -	if (count < sizeof(unsigned long))
> +	if ((sizeof(int) != sizeof(long) && count < sizeof(compat_ulong_t)) ||
> +	    (sizeof(int) == sizeof(long) && count < sizeof(unsigned long)))
>  		return -EINVAL;
> 
>  	add_wait_queue(&devp->hd_waitqueue, &wait);
> @@ -294,9 +295,15 @@ hpet_read(struct file *file, char __user *buf, 
> size_t count, loff_t * ppos)
>  		schedule();
>  	}
> 
> -	retval = put_user(data, (unsigned long __user *)buf);
> -	if (!retval)
> -		retval = sizeof(unsigned long);
> +	if (sizeof(int) != sizeof(long) && count == sizeof(compat_ulong_t)) {
> +		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);
> +	}

This does not look right: you are changing the behavior for
both 32-bit and 64-bit mode, and now choose the output based
on how many bytes userspace asked for. This has at least two
problems:

- if userspace asks for 5 to 7 bytes on a 64-bit kernel,
  it returns 8 bytes, overflowing the provided buffer.
- when 32-bit userspace asks for 8 or more bytes, it
  gets 8 bytes instead of 4.

Instead, you should check in_compat_syscall() to see
which output matches the running task.

> @@ -651,14 +658,23 @@ struct compat_hpet_info {
>  	unsigned short hi_timer;
>  };
> 
> +#define COMPAT_HPET_INFO       _IOR('h', 0x03, struct compat_hpet_info)
> +#define COMPAT_HPET_IRQFREQ    _IOW('h', 0x6, compat_ulong_t)

Right, it looks this was missing from my original change 54066a57c584
("hpet: kill BKL, add compat_ioctl").

COMPAT_HPET_IRQFREQ should probably have a comment about the
command code being wrong.

>  	mutex_lock(&hpet_mutex);
> -	err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
> +	err = hpet_ioctl_common(file->private_data, cmd, (unsigned 
> long)compat_ptr(arg), &info);
>  	mutex_unlock(&hpet_mutex);

While this works on x86, this is not a correct use of
compat_ptr() on architectures that modify the pointer
in compat_ptr(). Since HPET_INFO is the only one that
passes a pointer and that one is handled separately,
you should just drop the compat_ptr() conversion here.

    Arnd

Re: [PATCH] hpet: Support 32-bit userspace

[He Zhe]

On 2024/6/6 16:12, Arnd Bergmann wrote:
> On Thu, Jun 6, 2024, at 08:13, He Zhe wrote:
>> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
>> index d51fc8321d41..025a5905a370 100644
>> --- a/drivers/char/hpet.c
>> +++ b/drivers/char/hpet.c
>> @@ -269,7 +269,8 @@ hpet_read(struct file *file, char __user *buf, 
>> size_t count, loff_t * ppos)
>>  	if (!devp->hd_ireqfreq)
>>  		return -EIO;
>>
>> -	if (count < sizeof(unsigned long))
>> +	if ((sizeof(int) != sizeof(long) && count < sizeof(compat_ulong_t)) ||
>> +	    (sizeof(int) == sizeof(long) && count < sizeof(unsigned long)))
>>  		return -EINVAL;
>>
>>  	add_wait_queue(&devp->hd_waitqueue, &wait);
>> @@ -294,9 +295,15 @@ hpet_read(struct file *file, char __user *buf, 
>> size_t count, loff_t * ppos)
>>  		schedule();
>>  	}
>>
>> -	retval = put_user(data, (unsigned long __user *)buf);
>> -	if (!retval)
>> -		retval = sizeof(unsigned long);
>> +	if (sizeof(int) != sizeof(long) && count == sizeof(compat_ulong_t)) {
>> +		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);
>> +	}
> This does not look right: you are changing the behavior for
> both 32-bit and 64-bit mode, and now choose the output based
> on how many bytes userspace asked for. This has at least two
> problems:
>
> - if userspace asks for 5 to 7 bytes on a 64-bit kernel,
>   it returns 8 bytes, overflowing the provided buffer.
> - when 32-bit userspace asks for 8 or more bytes, it
>   gets 8 bytes instead of 4.
>
> Instead, you should check in_compat_syscall() to see
> which output matches the running task.

Thanks. I have should have known this helper.

v2 sent to address the above and below issues.

Regards,
Zhe

>
>> @@ -651,14 +658,23 @@ struct compat_hpet_info {
>>  	unsigned short hi_timer;
>>  };
>>
>> +#define COMPAT_HPET_INFO       _IOR('h', 0x03, struct compat_hpet_info)
>> +#define COMPAT_HPET_IRQFREQ    _IOW('h', 0x6, compat_ulong_t)
> Right, it looks this was missing from my original change 54066a57c584
> ("hpet: kill BKL, add compat_ioctl").
>
> COMPAT_HPET_IRQFREQ should probably have a comment about the
> command code being wrong.
>
>>  	mutex_lock(&hpet_mutex);
>> -	err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
>> +	err = hpet_ioctl_common(file->private_data, cmd, (unsigned 
>> long)compat_ptr(arg), &info);
>>  	mutex_unlock(&hpet_mutex);
> While this works on x86, this is not a correct use of
> compat_ptr() on architectures that modify the pointer
> in compat_ptr(). Since HPET_INFO is the only one that
> passes a pointer and that one is handled separately,
> you should just drop the compat_ptr() conversion here.
>
>     Arnd