linux-kernel - Re: [PATCH] posix-clock: drop code duplication using compat_ptr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <31acd2d0-8384-4ec5-acaa-187323b1968d@t-8ch.de>
Date: Mon, 20 Jan 2025 23:41:26 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Cyrill Gorcunov <gorcunov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>, 
	Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] posix-clock: drop code duplication using compat_ptr_ioctl

Hi Cyrill,

On 2025-01-21 01:30:11+0300, Cyrill Gorcunov wrote:
> On Mon, Jan 20, 2025 at 11:22:52PM +0100, Thomas Weißschuh wrote:
> ...
> > > -#ifdef CONFIG_COMPAT
> > > -	.compat_ioctl	= posix_clock_compat_ioctl,
> > > -#endif
> > > +	.compat_ioctl	= compat_ptr_ioctl,
> > 
> > This is not correct on s390. (It wasn't before either, though)
> > The improved patch below is in my personal queue, but I didn't get
> > around to actually testing and submitting it yet.
> 
> Hi, Thomas! Thanks for looking into the patch! I somehow miss why it won't
> work 'cause compat_ptr_ioctl already does the same conversion as in your
> code below, no? I miss something obvious?

See below.

> 
> ```
> long compat_ptr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> 	if (!file->f_op->unlocked_ioctl)
> 		return -ENOIOCTLCMD;
> 
> 	return file->f_op->unlocked_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> }
> EXPORT_SYMBOL(compat_ptr_ioctl);
> ```
> 
> > +#ifdef CONFIG_COMPAT
> > +long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
> > +		      unsigned long arg)
> > +{
> > +	switch (cmd) {
> > +	case PTP_ENABLE_PPS:
> > +	case PTP_ENABLE_PPS2:
> > +		/* These take in scalar arg, do not convert */
> > +		break;
> > +	default:
> > +		arg = (unsigned long)compat_ptr(arg);
> 
> Here^^^

The key is to only call compat_ptr() on *pointers*.
Scalars have to be passed through unmodified.
For ptp_ioctl(), PTP_ENABLE_PPS and PTP_ENABLE_PPS2 take such scalars,
which is why those two *can not* use compat_ptr().
compat_ptr_ioctl() however passes all arguments through compat_ptr().

Admittedly it's quite unlikely anybody would pass a value where it would
make a difference in practice. But if we fix this now, it might as well
be correct.

> 
> > +	}
> > +
> > +	return ptp_ioctl(pccontext, cmd, arg);
> > +}
> > +#endif