Received-SPF: pass (google.com: domain of linux-kernel+bounces-203805-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-497-g97f96844c-fm-20240526.001-g97f96844
Precedence: bulk
MIME-Version: 1.0
Message-Id: <acfe6adb-2070-445c-aecc-ec0972355d8a@app.fastmail.com>
In-Reply-To: <20240606061342.2142527-1-zhe.he@windriver.com>
References: <20240606061342.2142527-1-zhe.he@windriver.com>
Date: Thu, 06 Jun 2024 10:12:45 +0200
From: "Arnd Bergmann" <arnd@arndb.de>
To: "He Zhe" <zhe.he@windriver.com>, "Clemens Ladisch" <clemens@ladisch.de>,
 "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hpet: Support 32-bit userspace
Content-Type: text/plain

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