Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6963 bytes --]

On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > >
> > > > > > Hi Dmitry!
> > > > > >
> > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > another argument why not specify 'char *' in _IOW:
> > > > > >
> > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > >
> > > > > > So e.g.:
> > > > > >
> > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > >
> > > > > > is called from userspace as:
> > > > > >
> > > > > >   int val;
> > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > >
> > > > > > Or
> > > > > >
> > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > >
> > > > > > is called as:
> > > > > >
> > > > > >   struct input_mask val;
> > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > >
> > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > struct input_mask, but struct input_mask itself.
> > > > > >
> > > > > > And in case you define
> > > > > >
> > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > >
> > > > > > then you by above usage you should pass data as:
> > > > > >
> > > > > >   char *val = "DATA";
> > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > >
> > > > > > Which is not same as just:
> > > > > >
> > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > >
> > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > case you passed only pointer to data.
> > > > > >
> > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > also reason why compat ioctl for it was introduced.
> > > > >
> > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > what to do with all of this...
> > > > >
> > > > > Maybe we should define
> > > > >
> > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > >
> > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > so functions/macros do not take buffer length.
> > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > 
> > This is something different as for these functions you pass buffer and
> > length of buffer which is used in write mode -- not for read mode.
> > 
> > > the user to kernel boundary of ioctl, I think we should require size
> > > of the user buffer regardless of the data type.
> > > 
> > > > _STR therefore in names looks inconsistent.
> > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > requiring the length seems to be common across various ioctls.
> > > * input.h requires a length when requesting the phys and uniq
> > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > * Same with HIDRAW when setting and getting features:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > 
> > All these ioctls where is passed length are in opposite direction
> > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > 
> > I fully agree that when you need to read something from kernel
> > (_IOC_READ) and then writing it to userspace, you need to specify length
> > of userspace buffer. Exactly same as with userspace functions like
> > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > overflow as callee does not know length of buffer.
> > 
> > But here we we have there quite different problem, we need to write
> > something to kernel from userspace (_IOC_WRITE) and we are passing
> > nul-term string. So in this case specifying size is not required as it
> > is implicitly specified as part of passed string.
> 
> With the new IOCTL definitions it does not need to be a NULL-terminated
> string. It can be a buffer of characters with given length, and kernel
> will NULL-terminate as this it what it wants, not what the caller has to
> give.

Hi Dmitry! I was thinking more about this problem and I will propose
alternative solution, but first with details...

I think that we should use NULL terminated strings. Or better disallow
NULL byte inside string. Reason: all userspace application expects that
input device name would be NULL terminated which implies that in the
middle of name cannot be NULL byte.

So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
ioctl API to use buffer + size (with upper bound limit for size) instead
of passing NULL term string (with upper bound limit for string size)
then kernel have to add a leading NULL byte to string, plus check that
in the buffer there is no NULL byte. I guess this a very little
complicate code, but nothing which is problematic.

And on the userspace part. Now when userspace want to pass constant
string for device name, it just call

  ioctl(fd, UI_SET_PHYS, "my name of device");

After adding a new ioctl with buffer + size API, userspace would have to
call:

  ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");

which looks strange, so programmers would had to move device name into
new variable:

  const char *name = "my name of device";
  ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);

For me the old ioctl API looks easier to use (no need for strlen() or
extra variable), but this is just my preference of usage -- as it is
simpler for me. Maybe you would have different opinion...

And now question: Why we have uinput_compat_ioctl()? It is there only
because size part of IOCTL number is different on 32bit and 64bit
systems. As we know size part of UI_SET_PHYS is wrong and does not make
sense...

Would not it be better to change size of UI_SET_PHYS to just zero and
then when matching ioctl number just ignore size for this UI_SET_PHYS
ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
added in: https://git.kernel.org/tip/tip/c/7c7da40

And we would not have to deal with uinput_compat_ioctl() at all.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]