Bug 766 - ioctl() incorrectly decodes argument
Summary: ioctl() incorrectly decodes argument
Status: NEW
Alias: None
Product: glibc
Classification: Unclassified
Component: hurd (show other bugs)
Version: 2.3.6
: P2 normal
Target Milestone: ---
Assignee: Roland McGrath
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-02-25 18:32 UTC by Samuel Thibault
Modified: 2018-04-20 13:33 UTC (History)
3 users (show)

See Also:
Host: i686-unknown-gnu0.3
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Proposed patch (686 bytes, patch)
2005-07-27 23:15 UTC, Samuel Thibault
Details | Diff
testcase (was failing) (114 bytes, text/plain)
2005-07-27 23:17 UTC, Samuel Thibault
Details
Testcase (still works) (126 bytes, text/plain)
2005-07-27 23:18 UTC, Samuel Thibault
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel Thibault 2005-02-25 18:32:20 UTC
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
Comment 1 Samuel Thibault 2005-07-27 23:15:23 UTC
Created attachment 561 [details]
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).
Comment 2 Samuel Thibault 2005-07-27 23:17:43 UTC
Created attachment 562 [details]
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.
Comment 3 Samuel Thibault 2005-07-27 23:18:51 UTC
Created attachment 563 [details]
Testcase (still works)

This testcase checks that ioctls continue to work, event "1 integer passed via
pointer" ones.
Comment 4 Samuel Thibault 2005-07-27 23:33:22 UTC
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
Comment 5 Samuel Thibault 2005-08-11 01:20:47 UTC
Comment on attachment 561 [details]
Proposed patch

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.
Comment 6 Samuel Thibault 2005-11-23 16:47:03 UTC
Any progress on this issue? 
Comment 7 Roland McGrath 2006-02-05 22:43:49 UTC
updated version, still outstanding post-2.3.6
Comment 8 Thomas Schwinge 2012-05-11 02:54:06 UTC
6753048948b86f3b045710f77e1616b348562fa9

Is the test case worth being added, too?
Comment 9 Roland McGrath 2012-05-11 17:00:26 UTC
Having regression cases in the libc test suite is always a good idea.
Comment 10 Jackie Rosen 2014-02-16 18:28:44 UTC Comment hidden (spam)