On Wednesday 18 December 2019 12:26:59 Pali Rohár wrote:

On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:

quoted

On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:

quoted

On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:

quoted

On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:

quoted

On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár [off-list ref] wrote:

quoted

On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:

quoted

On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:

quoted

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.

quoted

the user to kernel boundary of ioctl, I think we should require size
of the user buffer regardless of the data type.

quoted

_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.

Below is example how change for removing UI_SET_PHYS_COMPAT may look
like. As header file is not changed and UI_SET_PHYS accepts any size
argument, it therefore accept also 32bit and 64bit integer. So no
existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
not be broken...

Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
number? Because introduction of new IOCTL number has one big
disadvantage: Userspace applications needs to support fallback to old
number as older versions of kernels would be in use for a long time. And
because kernel does not have to remove old IOCTL number for backward
compatibility there is basically no need for userspace application to
user new UI_SET_PHYS_STR IOCTL number...

Hello! I would like to remind this discussion as problem around a new
UI_SET_UNIQ ioctl is not solved yet and uniq property is really useful
for e.g. bluetooth (uinput) devices.

Dmitry, when you have a time, could you please look at this discussion
and decide how to go ahead?

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