On Tue, Nov 01, 2011 at 09:07:11AM +0200, Mikolaj Golub wrote: > > On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote: > > KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg > and > KB> env vector sizes. This can easily cause kernel panics due to unability to > KB> malloc the requested memory. I recommend to put some clump, and twice > KB> of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in > particular, > KB> exec_alloc_args). Also, you might use the swappable memory for the > strings > KB> as well, in the style of exec_alloc_args(). > > After looking at it more closely, I am not sure if I need to use > exec_alloc_args. I malloc explicitly only for array vector (proc_vector). And > actually it should be much smaller than 2 * (PATH_MAX + ARG_MAX). Currently in > linprocfs the limit is 512 entries: > > #define MAX_ARGV_STR 512 /* Max number of argv-like strings */ > > The same limit is in libkvm: > > /* > * Check that there aren't an unreasonable number of arguments, > * and that the address is in user space. Special test for > * VM_MIN_ADDRESS as it evaluates to zero, but is not a simple zero > * constant for some archs. We cannot use the pre-processor here and > * for some archs the compiler would trigger a signedness warning. > */ > if (narg > 512 || addr + 1 < VM_MIN_ADDRESS + 1 || addr >= > VM_MAXUSER_ADDRESS) > return (0); > > (BTW, may be the VM_MIN_ADDRESS - VM_MAXUSER_ADDRESS is worth adding in my > code too?) > > So it looks like I should use the same limit (512 * sizeof(char *)) for the > allocated array. I could use exec_alloc_args() for the allocation but it would > reqire some changes: I would have to free using kmem_free_wakeup(), which > requires size of the region, while I return the number of entries. So I'd > rather not use exec_alloc_args() for vector allocation because the benefit is > not significant here. > > For strings I use sbuf and set it up using sbuf_new_for_sysctl. I could set it > up manually as SBUF_FIXEDLEN allocating buf (up to 2 * (PATH_MAX + ARG_MAX)) > with exec_alloc_args() but this would complicate things a little. Do you think > it is worth doing? I mean using the swappable memory for strings, i.e. for the data you are currently store in sbuf. It indeed may be tricky, it was only an idea.
pgp6emnWdCNQf.pgp
Description: PGP signature
