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:

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.

Thanks.

-- 
Dmitry