Added PR_SET_PROCTITLE_AREA option for prctl()
in [Kernel]
|
Prev: pciehp_driver.c: PCIe hot plug for Intel iommu - call IOMMU API in hot remove
Next: 2.6.31 regression sis5513 PIO Mode 0 hang
From: Bryan Donlan on 2 Oct 2009 22:10 On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <tss(a)iki.fi> wrote: > PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the > given pointers, which makes it possible for user space to implement > setproctitle(3) cleanly. > @@ -267,9 +267,12 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer) > > � � � �res = access_process_vm(task, mm->arg_start, buffer, len, 0); > > - � � � // If the nul at the end of args has been overwritten, then > - � � � // assume application is using setproctitle(3). > - � � � if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > + � � � if (mm->arg_end != mm->env_start) { > + � � � � � � � // PR_SET_PROCTITLE_AREA used > + � � � � � � � res = strnlen(buffer, res); Is this check really needed? Surely it's enough to simply state that behavior if the area isn't null-terminated is undefined. > + � � � } else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > + � � � � � � � // If the nul at the end of args has been overwritten, then > + � � � � � � � // assume application is using old style setproctitle(3). > � � � � � � � �len = strnlen(buffer, res); > � � � � � � � �if (len < res) { > � � � � � � � � � �res = len; Might want to fix the bug later on in that function while you're in here - the second access_process_vm call is never checked for errors, but (from my reading) it's possible that the page that the environment is on could be unmapped between those two calls. The result could either be a short read (not the end of the world) or a negative value (error code + small original argument length) passed to strnlen. That said, come to think of it, I'm not actually sure if this prctl stuff is strictly necessary. Wouldn't it be enough for glibc to copy the environment somewhere safe, and then have the kernel guarantee a full PAGE_SIZE between arg_start and env_end, even if this means padding out the environment? The process could then measure to make sure it has this much space (in case of running on an old kernel) by testing the difference between arg_start and the top of the stack, or an auxiliary vector could be passed down from the kernel with the maximum proctitle length. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Timo Sirainen on 2 Oct 2009 22:50 On Oct 2, 2009, at 10:01 PM, Bryan Donlan wrote: > On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <tss(a)iki.fi> wrote: >> PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the >> given pointers, which makes it possible for user space to implement >> setproctitle(3) cleanly. > > >> @@ -267,9 +267,12 @@ static int proc_pid_cmdline(struct task_struct >> *task, char * buffer) >> >> res = access_process_vm(task, mm->arg_start, buffer, len, 0); >> >> - // If the nul at the end of args has been overwritten, then >> - // assume application is using setproctitle(3). >> - if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { >> + if (mm->arg_end != mm->env_start) { >> + // PR_SET_PROCTITLE_AREA used >> + res = strnlen(buffer, res); > > Is this check really needed? Surely it's enough to simply state that > behavior if the area isn't null-terminated is undefined. Well, that depends. I was hoping to use the syscall only once per process. That would allow me to just update the process title whenever I feel like it, possibly hundreds of times per second. This is much cheaper if I don't have to use a syscall every time. So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory area, without the above code ps will show it entirely regardless of any \0 characters (because parameters are separated by \0). >> + } else if (res > 0 && buffer[res-1] != '\0' && len < >> PAGE_SIZE) { >> + // If the nul at the end of args has been >> overwritten, then >> + // assume application is using old style >> setproctitle(3). >> len = strnlen(buffer, res); >> if (len < res) { >> res = len; > > Might want to fix the bug later on in that function while you're in > here - the second access_process_vm call is never checked for errors, > but (from my reading) it's possible that the page that the environment > is on could be unmapped between those two calls. The result could > either be a short read (not the end of the world) or a negative value > (error code + small original argument length) passed to strnlen. Hmm. Originally I thought it would have returned only -1, but if it's - errno then I'm beginning to wonder if this is a security hole. If the original res is small enough, and it looks like it can be, that code could get res to negative, i.e. unlimited. But I can't follow the code right now if it also means that userspace can read tons of data or if it gets caught by some "< 0" check. > That said, come to think of it, I'm not actually sure if this prctl > stuff is strictly necessary. Wouldn't it be enough for glibc to copy > the environment somewhere safe, and then have the kernel guarantee a > full PAGE_SIZE between arg_start and env_end, even if this means > padding out the environment? The process could then measure to make > sure it has this much space (in case of running on an old kernel) by > testing the difference between arg_start and the top of the stack, or > an auxiliary vector could be passed down from the kernel with the > maximum proctitle length. This would get around the potential "not enough space" problem, but not really the ugliness. I can't really think of other potential problems with it right now, but my main concern is actually getting setproctitle() to glibc. Based on Ulrich's previous reply to me I don't know if he'd be willing to accept that solution: http://sources.redhat.com/ml/libc-alpha/2009-10/msg00000.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bryan Donlan on 2 Oct 2009 23:10 On Fri, Oct 2, 2009 at 10:47 PM, Timo Sirainen <tss(a)iki.fi> wrote: >>> - � � � // If the nul at the end of args has been overwritten, then >>> - � � � // assume application is using setproctitle(3). >>> - � � � if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { >>> + � � � if (mm->arg_end != mm->env_start) { >>> + � � � � � � � // PR_SET_PROCTITLE_AREA used >>> + � � � � � � � res = strnlen(buffer, res); >> >> Is this check really needed? Surely it's enough to simply state that >> behavior if the area isn't null-terminated is undefined. > > Well, that depends. I was hoping to use the syscall only once per process. > That would allow me to just update the process title whenever I feel like > it, possibly hundreds of times per second. This is much cheaper if I don't > have to use a syscall every time. > > So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory > area, without the above code ps will show it entirely regardless of any \0 > characters (because parameters are separated by \0). That makes sense - but note that it's not completely atomic still - with a syscall you could insert some kind of barrier (rwsem?) to ensure other processes don't see a halfway updated process name. With infrequent updates this isn't a problem, but if you're really intending to update it at a rate where syscall overhead becomes a problem, then you're also going to see these kinds of issues as well. > >>> + � � � } else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { >>> + � � � � � � � // If the nul at the end of args has been overwritten, >>> then >>> + � � � � � � � // assume application is using old style setproctitle(3). >>> � � � � � � � len = strnlen(buffer, res); >>> � � � � � � � if (len < res) { >>> � � � � � � � � � res = len; >> >> Might want to fix the bug later on in that function while you're in >> here - the second access_process_vm call is never checked for errors, >> but (from my reading) it's possible that the page that the environment >> is on could be unmapped between those two calls. The result could >> either be a short read (not the end of the world) or a negative value >> (error code + small original argument length) passed to strnlen. > > Hmm. Originally I thought it would have returned only -1, but if it's -errno > then I'm beginning to wonder if this is a security hole. If the original res > is small enough, and it looks like it can be, that code could get res to > negative, i.e. unlimited. But I can't follow the code right now if it also > means that userspace can read tons of data or if it gets caught by some "< > 0" check. By the time we get to the second read, len is definitely between 0 and PAGE_SIZE, so that's not a problem. What I'm worried about mostly is whether strnlen would interpret its argument (negative error + small positive value = negative value) as a large positive value, go running off into the woods and cause an OOPS. Which I suppose is a denial of service issue. That said, I'm not really sure why it's written the way it is anyway. Why not just unconditionally try to load all the way from arg_start to env_end (or to arg_start + PAGE_SIZE), then just figure out where the \0 is and you're done? It would seem to have the same effect... >> That said, come to think of it, I'm not actually sure if this prctl >> stuff is strictly necessary. Wouldn't it be enough for glibc to copy >> the environment somewhere safe, and then have the kernel guarantee a >> full PAGE_SIZE between arg_start and env_end, even if this means >> padding out the environment? The process could then measure to make >> sure it has this much space (in case of running on an old kernel) by >> testing the difference between arg_start and the top of the stack, or >> an auxiliary vector could be passed down from the kernel with the >> maximum proctitle length. > > This would get around the potential "not enough space" problem, but not > really the ugliness. I can't really think of other potential problems with > it right now, but my main concern is actually getting setproctitle() to > glibc. Based on Ulrich's previous reply to me I don't know if he'd be > willing to accept that solution: > http://sources.redhat.com/ml/libc-alpha/2009-10/msg00000.html I don't think it's a particularly ugly solution - all it does is canonizes what programs are _already doing_, while at the same time giving more precise and useful guarantees. This means every old program which might, in theory, have been unreliable previously (due to assuming that its argument vector would be long enough) is now magically safe. And since if all goes well it'll get wrapped up in a nice C library wrapper eventually anyway, minor low-level ugliness shouldn't be a problem... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Timo Sirainen on 2 Oct 2009 23:30 On Oct 2, 2009, at 10:59 PM, Bryan Donlan wrote: >> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB >> memory >> area, without the above code ps will show it entirely regardless of >> any \0 >> characters (because parameters are separated by \0). > > That makes sense - but note that it's not completely atomic still - > with a syscall you could insert some kind of barrier (rwsem?) to > ensure other processes don't see a halfway updated process name. With > infrequent updates this isn't a problem, but if you're really > intending to update it at a rate where syscall overhead becomes a > problem, then you're also going to see these kinds of issues as well. I'm not worried about atomicity. Typically programs (like mine) would still show their executable name first and then followed by human readable status text. If the human readable text is corrupted once in a while (and practically never), I doubt anyone cares. In my case it would be an IMAP server and I was just thinking of maybe updating the process title to show what command is being executed (or some other long running task is going on). Then if there's a load problem or something the admin could easily check what some process that's been stuck for several minutes is doing. So it's not important to show the process title for those that rapidly change it, but for those that have been stuck for a while. >>> Might want to fix the bug later on in that function while you're in >>> here - the second access_process_vm call is never checked for >>> errors, >>> but (from my reading) it's possible that the page that the >>> environment >>> is on could be unmapped between those two calls. The result could >>> either be a short read (not the end of the world) or a negative >>> value >>> (error code + small original argument length) passed to strnlen. >> >> Hmm. Originally I thought it would have returned only -1, but if >> it's -errno >> then I'm beginning to wonder if this is a security hole. If the >> original res >> is small enough, and it looks like it can be, that code could get >> res to >> negative, i.e. unlimited. But I can't follow the code right now if >> it also >> means that userspace can read tons of data or if it gets caught by >> some "< >> 0" check. > > By the time we get to the second read, len is definitely between 0 and > PAGE_SIZE, so that's not a problem. What I'm worried about mostly is > whether strnlen would interpret its argument (negative error + small > positive value = negative value) as a large positive value, Right. > go running > off into the woods and cause an OOPS. Which I suppose is a denial of > service issue. I was more thinking that strnlen() would see \0, but not before it had returned some sensitive information that shouldn't have been returned to userspace. > That said, I'm not really sure why it's written the way it is anyway. > Why not just unconditionally try to load all the way from arg_start to > env_end (or to arg_start + PAGE_SIZE), then just figure out where the > \0 is and you're done? It would seem to have the same effect... I guess some minor performance benefit, since typically programs won't change their process title so the second access is rare. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: matthieu castet on 3 Oct 2009 07:20
Timo Sirainen <tss <at> iki.fi> writes: > > PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the > given pointers, which makes it possible for user space to implement > setproctitle(3) cleanly. > > Why can't you use PR_SET_NAME ? And with PR_SET_NAME, you can a name per thread, which you can't do by changing arg[0] name (it should be shared across all thread). Matthieu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |