Is PTYDevice::Ioctl() a possible security/stability problem?

[robertlipe]

Hi. I'm not actually a LemonOS user. I'm just cruising around reading OS code and I like a lot of what I see in LemonOS - congrats. While I'm thus not a LemonOS dev, I have lots of experience in UNIX-like kernels. Sometimes that's helpful and sometimes it can lead me astray. :-)

There is one bit of code that catches my eye as potentially problematic. It's possible there's something in place elsewhere to prevent this, but it looks like the arg in PTYDevice::Ioctl() is treated as a kernel mode pointer by all five subcommands. I suspect that an evil caller of these, especially from user space, could use these to (slowly) read and write arbitrary kernel memory. If so, I'm sure you recognize that as a Bad Thing.

In a traditional UNIX system, there's a possibility that the pointers could be to user space that's paged out or in another process or in kernel space(!). We'd protect against that via copyin() and copyout()() and testing for errors. I think the LemonOS spelling of that is approximately:
if(!Memory::CheckUsermodePointer(arg, sizeof(THING), currentProcess->addressSpace)){
return -EFAULT;
}

Even if arbitrary read/writes aren't possible (I can't tell if tios and wSz are exposed to user space) it does seem like just casting a pointer received from user space may result in crashes, especially if your OS isn't totally identity mapped with kernel and user addresses mapped the same. (And if they are, you probably have bigger security issues.)

Additionally, are there atomicity issues where termios or wsz in the base class can be partially updated by another process while we get put on the run queue and do these ioctls and get part of an old copy and part of a new copy because no lock is (visibly) held here? Maybe a lock is held externally at some higher external layer that I just don't see looking at pty.[cc,h] in isolation.

I could be wrong and this might all be totally safe. If I am wrong, you don't need to write a dissertation to educate me, but please at least look at that code and pencil-whip it to be sure it's sensible. (I know all too well that in large, complicated products - open and otherwise - sometimes error checking falls on the floor and we assume our callers aren't hostile. It happens.)

Cool project! Thanx for creating code that's easy to read, has a sane mix of C and C++, and no more asm() than it has to be.

[fido2020]

Thanks! Yeah unfortunately many of the syscalls and vfs calls do unchecked reads and writes to userspace. I am however working on a rewrite of the syscalls and I am also rewriting much of the vfs in the api-rework branch, which aims to address issues like this.

[robertlipe]

Spiffy. Sounds like the problem is real, that you know about it, and you have a plan to tackle it. Feel free to either close this report (no new information) or leave it open as a reminder that there's work to do in this area, including an audit of other related areas. Locking those structs when you do the copyin/out (assuming you don't have a BKL or just locks around all the system calls) is the kind of thing that can "work right" for years before some combination of faster processes anad changed workloads bring them to a head. FWIW, of all the projects listed in some recent "250 awesome OS projects", LemonOS is probably closest to what I'd have done given the need and the time. I've bookmarked you so that when my current projects wind down, I'll come back and look harder. Thanx for being awesome and good luck!

…

Message ID: ***@***.***>