On Sun, Nov 18, 2012 at 10:57 PM, P J P <ppandit@xxxxxxxxxx> wrote: > +-- On Sun, 18 Nov 2012, Kees Cook wrote --+ > | This is the second problem. I view this as less critical because it's only > | 64 instead of 4, but it certainly should be solved as well. > > I don't mean to be rude, but the patch I had sent solves both of these > problems with much less performance hit. > > Please see -> https://lkml.org/lkml/2012/10/26/442 > > Worst case: instead of 2^6(64) recursions, it would make only 4 calls to > request_module() function. find_module() does not resolve module aliases, it > could be removed from the above patch. > > Please pardon me if I came across rude or offensive, I'm only trying to make a > case. I don't think you're being rude at all. You're defending your solution. :) I wasn't very verbose in my objection to it, so I apologize for that and now attempt to make up for it in this email. :) I felt that your patch made certain things worse, but perhaps I was seeing it incorrectly. Just to have a common point of reference, here are some of the exec cases I've been considering. This is without either of our patches applied: An exec of ELF directly: - bprm created - search_binary_handler called - each fmt->load_binary called - ELF handler hits - done: 0 request_module calls, 0 leaks An exec of a a file handled by binfmt_misc: - bprm created - search_binary_handler called (depth == 0) - each fmt->load_binary called - misc handler hits - rewrites various bprm things, including bprm->interp - search_binary_handler called (depth == 1) - each fmt->load_binary called - ELF handler hits (usually, depends on specific binfmt_misc interpreter config) - done: 0 request_module calls, 0 leaks An exec of a script that calls ELF: - bprm created - search_binary_handler called (depth == 0) - each fmt->load_binary called - script handler hits - rewrites various bprm things, including bprm->interp - search_binary_handler called (depth == 1) - each fmt->load_binary called - ELF handler hits - done: 0 request_module calls, 0 leaks An exec of an binary with unprintables where a module exists: - bprm created - search_binary_handler called (depth == 0) - each fmt->load_binary called - nothing hits, retval == ENOEXEC - request_module("binfmt-%04x") called - search_binary_handler loop starts again - each fmt->load_binary called - new handler hits - done: 1 request_module call, 1 leak An exec of a chain of abusive scripts that contains unprintables: - bprm created - search_binary_handler called (depth == 0) - each fmt->load_binary called - script handler hits - rewrites various bprm things, including bprm->interp - search_binary_handler called (depth == 1) - each fmt->load_binary called - script handler hits - rewrites various bprm things, including bprm->interp - search_binary_handler called (depth == 2 ...) ... - each fmt->load_binary called - at depth > BINPRM_MAX_RECURSION, script handler returns -ENOEXEC - retval == ENOEXEC - request_module("binfmt-%04x") called - search_binary_handler loop starts again - each fmt->load_binary called - at depth > BINPRM_MAX_RECURSION, script handler returns -ENOEXEC ... unwind and retry continues all the way back up - retval == ENOEXEC - request_module("binfmt-%04x") called - search_binary_handler loop starts again - each fmt->load_binary called - done: 63 calls to request_module (one in each search_binary_handler call, to depth 5 (32 wide)), 63 leaks So, my interp-on-heap patch doesn't change the request_module call count, but it changes the stack reference leaks to kalloc/kfree pairs. Your patch changes a number of things. The obvious part is that is solves the abusive chain of script example by stopping at depth == 5, after 5 calls to request_module, and eliminates the retries so there are 0 stack reference leaks. However, it also changes the conditions for when a module is loaded (i.e. 0x7f no longer triggers a module_load, so anything needing that would break -- I'm not sure if this really qualifies for ABI breakage, I don't use any obscure binfmt modules so I can't say). And, most importantly, it triggers request_module for any binary with unprintables that binfmt_misc may already handle (for example, the very common case of handling DOS MZ files, which only define 2 bytes as magic (MZ) and exampes I find show things like "@\x00" trailing it, or JAR files which are PK\x03\x04). Which means each exec of these kinds of files would trigger a needless request_module() call on every exec. I feel that this too radically changes the behavioral characteristics of exec for common cases. The original logic is: - try all existing binfmt handlers - if none found, try to load a module for it in it had unprintables (including 0x7f) - try them all again Your new logic would be: - try to load a module if there are unprintables (excluding 0x7f) - try all existing binfmt handlers I really don't think this change is sensible. We need to only do the module request when none of the handlers hit, and things haven't gone wrong. I think to avoid the explosion of request_module calls in the abusive case, we could simply return ELOOP instead of ENOEXEC on max recursion. This would mean an immediate return on max depth failures. Also, after looking at the recursion tracking here, it really seems like search_binary_handler should track it, not the binfmt handlers. I'll send the patch. Both the interp-on-heap patch and this proposed ELOOP patch are needed to handle the case of binfmt_script and/or binfmt_misc being modules (first binfmt walk fails with -ENOEXEC, loads binfmt_script, retries loop, hits binfmt_script rewriting interp to a PE file, recurses, fails with -ENOEXEC, loads binfmt_misc via a modalias for PE files, retries loop, hits binfmt_misc rewriting interp to an ELF, recurses, loads ELF, happiness). Without the heap patch, we could be pointing into old stack (rewritten e.g. during module load or taking an interrupt, etc) on the loop retries. Without the ELOOP patch, the recursion could explode with an abusive script chain. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
