linux-kernel - Re: [PATCH v1 1/1] Input: ALPS - bail out when device path can't fit buffer

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] [day] [month] [year] [list]

Message-ID: <aBtk_zBywXqhU-YU@smile.fi.intel.com>
Date: Wed, 7 May 2025 16:49:51 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	Pali Rohár <pali@...nel.org>
Subject: Re: [PATCH v1 1/1] Input: ALPS - bail out when device path can't fit
 buffer

On Mon, May 05, 2025 at 10:34:56PM -0700, Dmitry Torokhov wrote:
> On Fri, May 02, 2025 at 04:52:51PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 29, 2025 at 08:01:10AM +0300, Andy Shevchenko wrote:
> > > Mon, Apr 28, 2025 at 04:30:13PM -0700, Dmitry Torokhov kirjoitti:
> > > > On Tue, Apr 22, 2025 at 09:56:45PM +0300, Andy Shevchenko wrote:

...

> > > > > +		n = snprintf(priv->phys2, sizeof(priv->phys2), "%s/input1",
> > > > > +			     psmouse->ps2dev.serio->phys);
> > > > > +		if (n >= sizeof(priv->phys2)) {
> > > > > +			psmouse_err(psmouse,
> > > > > +				    "failed to prepare path to the trackstick device\n");
> > > > > +			error = -E2BIG;
> > > > > +			goto init_fail;
> > > > 
> > > > So you just broke touchpad of some poor guy who had it working just fine 
> > > > for many years. For maximum impact you should add BUG() or panic()
> > > > here.
> > > 
> > > Ha-ha. You know that your speculation most likely so far from the truth.
> 
> If your code is not a noop that is precisely what happened.
> 
> > And actually what you are telling about is not true at all. If the device was
> > working it means that the file node name is not cut, and hence this patch won't
> > anyhow change this behaviour. Otherwise, provide an example which can fail this
> > and still be working in the user space.
> 
> "phys" is not a name of a device node. It is a string available via
> /proc/bus/input/devices, sysfs /sys/class/input/input<N>/phys and also
> EVIOCGPHYS ioctl. A driver is free to not set it at all and everything
> will be working fine.

Okay, this is then indeed a problematic in the cases when strings are shorten
than supposed to be.

> Actually, input devices themselves to not have device nodes, it is evdev
> interface that provides /dev/input/event<N>.
> 
> > > > In all seriousness, it is OK to have truncated phys, rarely anyone looks
> > > > at it and if we get a report of it being truncated then we can consider
> > > > addressing the size (or we can decide to live with it truncated).
> > > 
> > > In all seriousness, while I agree on the statement, the 4 drivers in Input
> > > subsystem break the build. It's the biggest obstacle now to enable WERROR=y,
> > > which is default, builds on `make W=1`. So, I already gave you chance to fix,
> > > instead I hear nothing back for a months (to be precise 2 months and a day
> > > passed from my first attempt that you didn't like), the problem still exists.
> > > Please, address this the way you like.
> > 
> > For the reference, the first approach:
> > https://lore.kernel.org/r/20250228121147.242115-1-andriy.shevchenko@linux.intel.com
> > where I also asked about this one, ano got no answer.
> 
> Sorry I was busy with other projects.
> 
> > I really don't want to try anything new as it seems a big pushback to whatever
> > I propose. So, please consider fixing the issues rather sooner. I will be more
> > than happy to test.
> 
> Have you considered that this warning is bogus and it should be disabled
> instead? Or maybe GCC should see if there are followup writes to the
> same buffer before emitting the warning? 

I considered this warning as a problem that prevents me compiling the code.
Since there are only few issues over the kernel left with some maintainers
who are definitely busy, I consider the disabling warning wouldn't make it
better. And if so, this should be send not by me, I have no good arguments
against it. Perhaps you have?

-- 
With Best Regards,
Andy Shevchenko