cvs commit: src/sys/dev/atkbdc atkbd.c src/sys/dev/digi digi.c
src/sys/dev/kbdmux kbdmux.c src/sys/dev/syscons scvidctl.c
syscons.c src/sys/dev/uart uart_kbd_sun.c src/sys/dev/usb
ukbd.c src/sys/dev/vkbd vkbd.c src/sys/fs/procfs procfs_ioctl.c ...
jhb at freebsd.org
Thu Sep 28 07:56:42 PDT 2006
On Wednesday 27 September 2006 18:12, Ruslan Ermilov wrote:
> On Wed, Sep 27, 2006 at 05:52:56PM -0400, John Baldwin wrote:
> > Could you avoid IOWINT by just assuming that any _IO() ioctl is getting an
> > as the arg?
> There are some _IO() ioctls that pass a pointer to variable sized data,
> and their ioctl handlers to uiocopy'ing rather than ioctl(). See
> sys/cam/scsi/scsi_ses.c, SESIOC_* ioctls for one such example.
I think these are just broken and should be fixed. :) SESIOC_ENCSTAT should
be IOW(..., ses_encstat) for example. The two troublesome ones GETNOBJ and
GETOBJMAP are broken by design. Somehow they need to tell userland how much
they copied out, and they also need to let userland specify the buffer length
to avoid buffer overflows. Thus, they need to be using a IOWR with a
structure containing a pointer and length (modify the length on return if
needed) to avoid problems anyway. I'd be all for just fixing the few that
abuse _IO to treat the arg as anything other than an int and assume _IO means
an int arg for 7.x and future. The solution for 6.x might be uglier, but
for the future we should fix the abusers and avoid adding the _IOWINT hack.
> > If an ioctl doesn't use the arg, then you don't lose anything..
> > do we have any ioctl's that use the arg directly but not as an int?
> Unfortunately yes.
Are there any others outside of SES? How many? If it's a small list, then
let's fix them. The SES ones are broken as an API anyway as mentioned above,
and if other ioctl's are copying out a variable amount of data w/o allowing
for buffer lengths or telling userland how much it copied, they are also
fundamentally broken as well.
> > The
> > ioctl(2) manpage implies that 'data' is either a pointer or an int. If
> > go this route, you avoid changing all the ioctl values, basically just
> > that IOC_VOID means the argument is an int.
> That has been considered and found impossible due to the above.
> We also don't have any spare bits left in the ioctl type field,
> so IOC_VOID with size == sizeof(int) have been used to implement
> _IOWINT(). IOC_VOID is incorrect name, the argument should either
> be a pointer or an "int", even when not used by ioctl(). Some
> ioctl() calls to "void" ioctls in userland don't pass a third
> argument. I think on architectures that pass arguments on the
> stack (such as i386) this causes return address to be accessed
> instead of the argument value. Ioctls that are "void" should
> either pass "0" or "NULL".
It will be stack (or register) garbage yes, but unread stack (or register)
More information about the cvs-src