Patch for buffer overrun in serial/console device logic

uml-devel Doug Dumitru 3 participants 8 messages Oct 7, 2003 – Nov 9, 2003 1 message not in the archive User-mode Linux (UML)

lore

Share & tools

lore

Doug Dumitru

author

2003-10-07 18:52 UTC #0 lore raw

I have been having trouble with buffer overruns when talking to pty devices. My boot config directs serial device with syntax like:

... ssl0=tty:/dev/pty/pty1009 ...

This sets up a serial line to talk with a pty device. We don't use the ptmx interface so that we can definitavely know which device to talk to from the outside.

The problem we are seeing is that outbound data gets overrun. If you send 100K out the stream, you tend to see the first 28K or so followed by a big gap followed by the last 3K or so of the stream. We have tested this with the ptmx syntax/logic and the problem still seems to be there. Also, if you test this with a very fast consumer (such as a local cat from the device), the problem does not manifest itself. UML must be able to produce data faster than the outside world can consume it.

After a lot of looking, it appears that the line_write routine in line.c does not check to see if the buffer has enough room. The buffer_data(...) routine does not appear to check for available space at all and just overwrites old data.

Here is the patch that appears to correct the issue:

um/drivers/line.c Tue Oct 7 10:27:06 2003 renamed from linux-2.4.22/arch/um/drivers/line.c Tue Oct 7 10:25:20 2003 +5 -0

@@ -97,6 +97,7 @@
        char *new;
        unsigned long flags;
        int n, err, i;
+       int room;
 
        if(tty->stopped) return 0;

@@ -115,6 +116,10 @@
        i = minor(tty->device) - tty->driver.minor_start;
        line = &lines[i];
 
+       room = line_write_room(tty);
+       if ( !room ) return -EAGAIN;
+       if ( room < len ) len = room;
+
        down(&line->sem);
        if(line->head != line->tail){
                local_irq_save(flags);

We have this running in our 2.4.22 build, but would like to see it (or something equivelent) in the release so that we can take patches cleanly.

We also have a much messier, in-house patch that we apply to hostfs.c so that root mounts will correctly honor ownerships and rights. Is anyone else working on this. The 2.4.20-5um release appears to have a lot of missing stuff in this area, but if someone else is re-working this, I will put my patch down for a while and wait. Otherwise, I will clean it up and submit it.

--------------------------------------------------------------------

Doug Dumitru     800-470-2756     (610-237-2000)
EasyCo LLC       [email protected]  http://easyco.com
--------------------------------------------------------------------
D3, U2, jBase Virtual Servers.    Off-site backup over the internet.
Develop/test/deploy from $20/mo.  Fast, secure, cheaper than tape.
http://mirroredservers.com        http://mirroredbackup.com

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Jeff Dike

2003-10-07 21:49 UTC #1 lore raw

Quoted 2 lines · [email protected] said:

We also have a much messier, in-house patch that we apply to hostfs.c so that root mounts will correctly honor ownerships and rights.

Does this involve some sort of list on the side which keeps track of ownership and permissions of the host files? If so, hostfs has needed something like this for a long time.

Quoted 1 line

Is anyone else working on this.

Not that I know of. It comes up every once in a while, but no one has actually written any code.

Jeff

Doug Dumitru

author

2003-10-07 22:28 UTC #2 lore raw

Quoted 11 lines · Jeff Dike

Quoted 2 lines · [email protected] said:
We also have a much messier, in-house patch that we apply to hostfs.c so that root mounts will correctly honor ownerships and rights.
Does this involve some sort of list on the side which keeps track of ownership and permissions of the host files? If so, hostfs has needed something like this for a long time.

Quoted 7 lines

Quoted 1 line
Is anyone else working on this.
Not that I know of. It comes up every once in a while, but no one has actually written any code.
Jeff

The in-house patch that we are running assumes that you boot UML as root, although I have considered a SUID root variant.

It looks at numeric UID/GIDs and maintains all of these all the way down to the user IO calls. Thus the files created in the hostfs have parellel UID/GID values and priviledges.

The code itself involves a lot of extra parameters from kernel to user space as things like the current user aren't propogated down. I personally think that our current patch set is "100% ugly" and would not consider posting it as-is. If people are interested in transparent numeric UID/GID to hostfs, then I would be happy to clean up what we have and submit it.

Our reason for doing the patch in-house is that we have added UML kernel code that supports an in-house application that causes hostfs to keep a "journel" of file IO transactions and posts this journel to a host pipe.
We then have host daemons (outside of the virtual) that move this
journel to a distant machine achieving remote filesystem replication. We use this for high-reliability mail and web services.

--------------------------------------------------------------------

Doug Dumitru     800-470-2756     (610-237-2000)
EasyCo LLC       [email protected]  http://easyco.com
--------------------------------------------------------------------
D3, U2, jBase Virtual Servers.    Off-site backup over the internet.
Develop/test/deploy from $20/mo.  Fast, secure, cheaper than tape.
http://mirroredservers.com        http://mirroredbackup.com

Jeff Dike

2003-10-11 01:46 UTC #3 lore raw

Quoted 6 lines · [email protected] said:

The code itself involves a lot of extra parameters from kernel to user
space as things like the current user aren't propogated down. I
personally think that our current patch set is "100% ugly" and would not consider posting it as-is. If people are interested in transparent numeric UID/GID to hostfs, then I would be happy to clean up what we have and submit it.

OK, that ain't the way to do it. Anything that involves passing a parallel set of creds through VFS will cause Al Viro to lop my head off. Since I'm somewhat attached to it, I will not propose such a thing, no matter how cleaned up it is.

What would work is to store the creds in a separate container of some sort on the host, and reference that inside hostfs when doing permission checks.

This is more or less what UMSDOS does, from what I understand, and it keeps the nastiness contained within hostfs.

That journalling is a neat idea, BTW.

Jeff

-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program. SourceForge.net hosts over 70,000 Open Source Projects. See the people who have HELPED US provide better services:
Click here: http://sourceforge.net/supporters.php
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Doug Dumitru

author

2003-10-12 03:36 UTC #4 lore raw

Quoted 10 lines · Jeff Dike

Quoted 6 lines · [email protected] said:
The code itself involves a lot of extra parameters from kernel to user space as things like the current user aren't propogated down. I personally think that our current patch set is "100% ugly" and would not consider posting it as-is. If people are interested in transparent numeric UID/GID to hostfs, then I would be happy to clean up what we have and submit it.

Quoted 4 lines

OK, that ain't the way to do it. Anything that involves passing a parallel set of creds through VFS will cause Al Viro to lop my head off. Since I'm somewhat attached to it, I will not propose such a thing, no matter how cleaned up it is.

There are actually a couple of sets of issues here.

1. The "_user" stuff needs to get to UID, GID, and UMASK.

2. When the "_user" stuff does IO, it should "become" the users in UID/GID. This way permissions "just work" and you don't have to clean anything up after the fact.

Addressing (1), I could always just transition a pointer to "current" and really open up a can of worms.

This breaks just about every rule of "object orientation", but on the other hand, file systems usually do have access to the process state, so maybe this is the "correct" approach.

Doing a seteuid(...) ... around the IO calls is actually pretty clean. I implemented it with a "change" / "unchange" set of helper routines and it pretty much worked first shot.

Quoted 9 lines

What would work is to store the creds in a separate container of some sort on the host, and reference that inside hostfs when doing permission checks.
This is more or less what UMSDOS does, from what I understand, and it keeps the nastiness contained within hostfs.
That journalling is a neat idea, BTW.
Jeff

--------------------------------------------------------------------

Doug Dumitru     800-470-2756     (610-237-2000)
EasyCo LLC       [email protected]  http://easyco.com
--------------------------------------------------------------------
D3, U2, jBase Virtual Servers.    Off-site backup over the internet.
Develop/test/deploy from $20/mo.  Fast, secure, cheaper than tape.
http://mirroredservers.com        http://mirroredbackup.com

BlaisorBlade

2003-10-13 20:43 UTC #5 lore raw

In reply to Jeff Dike

Alle 03:49, sabato 11 ottobre 2003, Jeff Dike ha scritto:

Quoted 13 lines

Quoted 6 lines · [email protected] said:
The code itself involves a lot of extra parameters from kernel to user
space as things like the current user aren't propogated down. I
personally think that our current patch set is "100% ugly" and would not consider posting it as-is. If people are interested in transparent numeric UID/GID to hostfs, then I would be happy to clean up what we have and submit it.
OK, that ain't the way to do it. Anything that involves passing a parallel set of creds through VFS will cause Al Viro to lop my head off. Since I'm somewhat attached to it, I will not propose such a thing, no matter how cleaned up it is.

Quoted 7 lines

What would work is to store the creds in a separate container of some sort on the host, and reference that inside hostfs when doing permission checks.
This is more or less what UMSDOS does, from what I understand, and it keeps the nastiness contained within hostfs.
That journalling is a neat idea, BTW.

This are my very humble opinions, just my 2 cents, however let's go. 1)a solution involving a setuid UML is just wrong. Something lighter is obviously welcome.
2) so, on the host, all files must have the same permissions, and the same owner, when you use hostfs as rootfs. I.e., the uid and the gid will have to be the same as the ones UML runs with. And the permission will have to be fixed ones. For instance, for files we have fumask=644 by default, and it can be set to anything not stricter than 600; for dirs, dumask=755 by default, and it mustn't be stricter than 700. If and only if you want to have fun, you can check if a file has u+x permission and give u+x(or a+x, it's matter of taste/security) on the host. These params can be set on the linux command line; a mount option can exist, but it must be "bounded" by the boot option(the same way you can specify the rootfs option and then restrict even more the mount).
Then, there can be UMSDOS-like stuff.
This is the only way to make possible things such as a file that inside UML has perms=000 and is readable by root. In this case, hostfs is a true filesystem.
3)But, there is a separate issue: accessing the host files, i.e. when you want to watch on files existing on the host(so you can't change freely their permission). This is a completely different case! Here you can't even suppose you can write the UMSDOS like permission-holding file.
So here root has limited permission. My idea here is to make things follow some extended semantic, like the 2.6 security models: neither there root has all powers, or seeming a NFS mount(neither there root has full permissions). It's just a start of idea, but *could* be interesting.

Signature · 16 lines

cat <<EOSIGN
Paolo Giarrusso, aka Blaisorblade
Linux Kernel 2.4.21/2.6.0-test on an i686; Linux registered user n. 292729
EOSIGN



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
SourceForge.net hosts over 70,000 Open Source Projects.
See the people who have HELPED US provide better services:
Click here: http://sourceforge.net/supporters.php
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

A message that is not in the archive (sent off-list or not yet received): p05111b00bba97b88a68d@[10.96.96.13]

Doug Dumitru

author

2003-10-08 16:22 UTC #7 lore raw

Quoted 7 lines · Hannes Schulz

[...]
Quoted 2 lines
We also have a much messier, in-house patch that we apply to hostfs.c so that root mounts will correctly honor ownerships and rights. Is anyone else working on this. The 2.4.20-5um release appears to have a lot of missing stuff in this area, but if someone else is re-working this, I will put my patch down for a while and wait. Otherwise, I will clean it up and submit it.
I did such a patch (see below). How does yours fix the problem ?
Unfortunately I have another problem with hostfs: munmap does not write out the changes to the file. I looked at the code but I am afraid this is beyound me.

[ ... snipped ... ]

My patch was quite a bit more involved.

The problem that I saw was that the actual IO operations in hostfs_user.c were always being executed as the user that start UML. In order to get all of the UID/GIDs to map transparently, you have to do setegid(...) and seteuid(...) operations around all of the IO operations that have the potential to create a file or directory. This includes open and a bunch of others. Unfurtunately, the calling UID/GID is never visible to the functons in hostfs_user.c, so hostfs_kern.c needs a new call (or new parameters) to pass this down. Umask also needs to travel down to hostfs_user.c.

My patch is very tangled in with my "transaction journel" that we use for replication. The replication stuff is very "non trivial" to implement from a setup point of view, so I don't want to give that to anyone at this point. Plus, the patch as stands has lots of debugging lines commented out and is just plain ugly. My reputation would not be served well to show it to anyone at this point. My real question was whether there is a need to more transparent UID/GID/UMASK processing to hostfs (with the caveat that UML must boot root or at least SUID root).

--------------------------------------------------------------------

Doug Dumitru     800-470-2756     (610-237-2000)
EasyCo LLC       [email protected]  http://easyco.com
--------------------------------------------------------------------
D3, U2, jBase Virtual Servers.    Off-site backup over the internet.
Develop/test/deploy from $20/mo.  Fast, secure, cheaper than tape.
http://mirroredservers.com        http://mirroredbackup.com

Jeff Dike

2003-11-09 01:45 UTC #8 lore raw

In reply to Doug Dumitru

Quoted 4 lines · [email protected] said:

After a lot of looking, it appears that the line_write routine in line.c does not check to see if the buffer has enough room. The buffer_data(...) routine does not appear to check for available space at all and just overwrites old data.

Indeed. Thanks for chasing this down. I fixed it, but in a different way.

Jeff

-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003, 16-19 November in Las Vegas. Learn firsthand the latest developments in Apache, PHP, Perl, XML, Java, MySQL, WebDAV, and more! http://www.apachecon.com/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel