[Bug hurd/766] New: ioctl() incorrectly decodes argument

[Bug hurd/766] New: ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

Hi,

This simple program:

#include <sys/ioctl.h>
#include <stdio.h>
int main(void)
{
  if (ioctl(1, TIOCDRAIN) < 0)
    perror("ioctl");
  return 0;
}

be it run by itself on any hurd's term-controlled console, always produces:
ioctl: (ipc/mig) server type check failure

While it just should work fine.

Tracing the ioctl call leads to glibc/sysdeps/mach/hurd/ioctl.c: __ioctl(), which builds a message and sends it to the "term" translator running on the console. No error here yet.

On the "term" translator, the message is received by hurd/term/tioctlServer.c: _Xtioctl_tiocdrain(), but the check:
        if ((In0P->Head.msgh_size != 24) ||
            (In0P->Head.msgh_bits & MACH_MSGH_BITS_COMPLEX))
                { OutP->RetCode = MIG_BAD_ARGUMENTS; return; }
fails because msgh_size actually is 32. This hence returns an error code, which ioctl() then returns to main().

The check doesn't seem to be wrong: this ioctl has no argument, so the message should indeed only hold the header (24 bytes) without any argument.

But tracing the building of the message in glibc/sysdeps/mach/hurd/ioctl.c:__ioctl(), in the nested send_rpc() function,
      if (_IOC_INOUT (request) & IOC_IN)
        {
	  ...
        }
      else if (_IOC_INOUT (request) == IOC_VOID)
        {
          /* The RPC takes a single integer_t argument.
             Rather than pointing to the value, ARG is the value itself.  */

          *t++ = io2mach_type (1, _IOTS (integer_t));
          *((integer_t *) t)++ = (integer_t) arg;
        }
the second statement is executed, packing some random arg in the message, getting its size to 32. The second if() seems odd to me: if _IOC_INOUT(request) is IOC_VOID, that means that the ioctl doesn't take any arg (see <bits/ioctls.h>), which is the case here, so none should be packed and the size would then be kept to 24 and everything would work (tested). This piece of code should only be called when ioctl() takes an argument which is not a pointer to some structure but the argument itself.

And actually, for now, if ioctl takes an argument which is not a pointer to some structure but the argument itself, the second statement is *not* executed, but rather the previous one, since _IOC_INOUT(request) would indeed hold IOC_IN (see <bits/ioctls.h>). And this statement segfaults (since it considers arg to be a pointer):

#include <sys/ioctl.h>
#include <stdio.h>
int main(void)
{
  if (ioctl(1,TIOCSETD,TTYDISC)<0)
    perror("ioctl");
  return 0;
}

This, run anyhow, just segfaults at
                  p = __mempcpy (p, argptr, len);
in the first statement.

The trouble seems to be that the argument coding of ioctl numbers in <bits/ioctls.h> is not precise enough to tell whether arg is the arg itself or a pointer to some structure, so that __ioctl() doesn't know whether to dereference arg or not.

Maybe 
#define _IOT_SIMPLE(type)       _IOT (_IOTS (type), 1, 0, 0, 0, 0)
could be turned into something like
#define _IOT_SIMPLE(type)       _IOT (0, 0, _IOTS (type), 1, 0, 0)
With the convention that if IOT_COUNT0(type)==0 but IOT_COUNT1(type)==1, arg is the argument itself.
Yes, this sucks, but there's not much room: count0 needs be 16 for struct ifreq, count1 needs be 20 for struct termios, count2 needs be 2 for struct termios.

BTW, further in __ioctl():
  va_list ap;
  va_start (ap, request);
  arg = va_arg (ap, void *);
  va_end (ap);
This should be if()ed by _IOC_INOUT(request) != IOC_VOID, since else the caller wouldn't have given any argument to ioctl() and va_arg() would at best return a random value, at worst crash.

Regards,
Samuel Thibault

-- 
           Summary: ioctl() incorrectly decodes argument
           Product: glibc
           Version: 2.3.2
            Status: NEW
          Severity: normal
          Priority: P2
         Component: hurd
        AssignedTo: roland at gnu dot org
        ReportedBy: samuel dot thibault at ens-lyon dot org
                CC: glibc-bugs at sources dot redhat dot com
  GCC host triplet: i686-unknown-gnu0.3


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-07-27 23:15 -------
Created an attachment (id=561)
 --> (http://sources.redhat.com/bugzilla/attachment.cgi?id=561&action=view)
Proposed patch

This patch corrects the no-parameter case, and add an _IOIW() ioctl declaration
macro for ioctls that would get value as immediates rather that by pointer (IO
Immediate Write).


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-07-27 23:17 -------
Created an attachment (id=562)
 --> (http://sources.redhat.com/bugzilla/attachment.cgi?id=562&action=view)
testcase (was failing)

This testcase was failing with this error message:
ioctl: (ipc/mig) server type check failure
With previously attached patch, it now works.


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-07-27 23:18 -------
Created an attachment (id=563)
 --> (http://sources.redhat.com/bugzilla/attachment.cgi?id=563&action=view)
Testcase (still works)

This testcase checks that ioctls continue to work, event "1 integer passed via
pointer" ones.


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-07-27 23:33 -------
Hi,

I attached a patch to correct the bug: it corrects the meaning of IOC_VOID /
IOC_IN / IOC_OUT:
- IOC_OUT / IOC_IN means that data is passed via a pointer (input/output/both ways);
- IOC_VOID means that either there is no data (_IOT_COUNT0 (type) == 0),
or the only data is an integer passed as an immediate value (_IOT_COUNT0
(type) == 1).

When (_IOT_COUNT0(type) == 0), that means there is no data, so
va_start/va_arg/va_end are now not even called, avoiding any random value
or even crash.

I looked through the list of hurd's ioctls, there is none that uses an
immediate argument, but since there is code to handle that case when building
the RPC, I guess it was yet considered to be possible. And indeed some other
systems sometimes define ioctl with immediate arguments: TCSBRK, TCXONC,
TCFLSH, TIOSCTTY, HDIO_SET_DMA & such, LPCHAR, ... So that I added an _IOIW()
macro to let people define such ioctl calls (IO Immediate Write).

Please ignore the "TIOCSETD segfaults" testcase in previous bug report:
of course the integer should be passed via a pointer in this case.

The two attached testcases work correctly with the patch applied.

Regards,
Samuel


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-08-11 01:20 -------
(From update of attachment 561)
2005-07-28  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* ioctl.c (__ioctl): Add handling of parameter-less ioctls.

2005-07-28  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* ioctls.h (_IOIW): New macro for immediate-write ioctls.


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[samuel dot thibault at ens-lyon dot org]

------- Additional Comments From samuel dot thibault at ens-lyon dot org  2005-11-23 16:47 -------
Any progress on this issue? 

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[roland at gnu dot org]

------- Additional Comments From roland at gnu dot org  2006-02-05 22:43 -------
updated version, still outstanding post-2.3.6

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|2.3.2                       |2.3.6


http://sourceware.org/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug hurd/766] ioctl() incorrectly decodes argument

[tschwinge at gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bug-hurd at gnu dot org


http://sourceware.org/bugzilla/show_bug.cgi?id=766

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.