> On Thu, Mar 10, 2016 at 08:48:21AM -0700, Theo de Raadt wrote: > > The reason for these checks is because they protect the kernel, > > and they identify a program that does the wrong thing. Here, a > > program did the wrong thing. I am 100% in agreement that opendev > > may not be the right place to do this. That kind of stems from > > the design of opendev regarding DUID conversion.... I think we all > > knew that wasn't the best design early on, but we needed to get > > that going, before the rest of the DUID subsystem could work... > > ???? > > If it's opendev that does the check, where should it be done ? Right now > krw added checks in both disklabel AND fdisk to prevent the issue from > coming up.
Is it ugly for a program to check it is doing an ioctl against a device before it does so? Perhaps. Perhaps we can delete the other 40 or so fstat calls in the tree which are done to verify these circumstances? Perhaps not. I don't have an answer for you. > BUT both those do opendev upfront, so it's really duplicated code. Hang on, I am not arguing against doing it in opendev(3). I don't where to do it. I will argue that what opendev(3) does - with ioctl DIOCMAP -- is non-atomic and ugly. But I recall why we did this, we needed a quick step to make DUIDs work, and this was the easiest way to do it. Maybe we are ready for a different way. So why is DIOCMAP whitelisted to a kill like that? ioctl is a system call backed by asstons of kernel code, essentially any ioctl operation can be attempted against any fd and will call through at minimum 2 layers of kernel code, probably maximum 7, before unwinding if the subsystem does not support that "32 bit request". So that is why pledge is so aggressively whitelisting, and blocks towards a kill -- so we can FIND THESE SITUATIONS rather than watching errors being ignored, so that we can identify problems, and learn how to adjust the entire system towards the new "subset of POSIX" frame of mind. Maybe this ioctl should return an error instead of killing. But we would not have known that this one should return an error, BEFORE the first instance reporting it. The handful of people who worked on the pledge kernel code just cannot anticipate everything, and 5.9 (and probably 6.0) will ship with a few issues like this. (this is a termite hill sized for 6 inhabitants) > The other thing that (in my opinion) makes sense would be to duplicate the > way ttys are handled, e.g., we've got a isatty library call that validates > an fd so that further tty(4) ioctl will work. Maybe this is what's needed, > or something similar. Hang on, isatty is a very special case. For those who don't know isatty(3) is backed by fcntl(2) using F_ISATTY, which is always allowed by pledge(2) with the "stdio" request, so it is not an accurate example. Moving it to fcntl(2) was a foundation of pledge... Yes in a few places we put a isatty(3) before a tty ioctl, but this is because the legacy code we were dealing with shares lower level functions between "file" and "tty" ops. The "tty" vs "stdout" abstraction is a big part of unix. DIOCMAP? Not so much. > I don't think leaving the check to the individual programs is a good solution. It is proposed for 2 programs now. There are 20+? users of opendev? I suggest checking a few more, then we'll know for sure. > I just stumbled on fdisk/disklabel being slightly broken by accident... after > a few months of pledge(2) testing. And what did you expect? It shows that there is insufficient testing of the entire utility base, to verify all semantics are correct. We are talking almost 800 programs, probably with argument choices there are 10,000 usage paterns. Some of the issues we fix in programs, some of them we fix in library routines, and some we fix in the pledge layer. We'll get there. I still don't know which this is, but /dev/diskmap + DIOCMAP is still a gross artifact.
