I cc'ed Linus and stable, but I don't know how to really test this patch
and I don't really understand the new creds management.
Please review.
load_flat_shared_library() does something strange (but hopefully this
patch doesn't break it). I do not understand why does it create the
new bprm. Afaics, it could reuse bprm pointer which comes as an argument
of ->load_binary(), all we need is to temporary change/restore bprm->file
for load_flat_file().
Oleg.
On 09/03, Oleg Nesterov wrote:
>
> load_flat_shared_library() does something strange (but hopefully this
> patch doesn't break it). I do not understand why does it create the
> new bprm. Afaics, it could reuse bprm pointer which comes as an argument
> of ->load_binary(), all we need is to temporary change/restore bprm->file
> for load_flat_file().
IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
plays with prepare_exec_creds().
But again, I don't understand this code, and I didn't even try to compile
this patch.
Oleg.
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -83,7 +83,7 @@ struct lib_info {
};
#ifdef CONFIG_BINFMT_SHARED_FLAT
-static int load_flat_shared_library(int id, struct lib_info *p);
+static int load_flat_shared_library(struct linux_binprm*, int, struct lib_info*);
#endif
static int load_flat_binary(struct linux_binprm *, struct pt_regs * regs);
@@ -308,7 +308,8 @@ out_free:
/****************************************************************************/
static unsigned long
-calc_reloc(unsigned long r, struct lib_info *p, int curid, int internalp)
+calc_reloc(struct linux_binprm *bprm, unnsigned long r, struct lib_info *p,
+ int curid, int internalp)
{
unsigned long addr;
int id;
@@ -335,7 +336,7 @@ calc_reloc(unsigned long r, struct lib_i
"(%d != %d)", (unsigned) r, curid, id);
goto failed;
} else if ( ! p->lib_list[id].loaded &&
- load_flat_shared_library(id, p) > (unsigned long) -4096) {
+ load_flat_shared_library(bprm, id, p) > (unsigned long) -4096) {
printk("BINFMT_FLAT: failed to load library %d", id);
goto failed;
}
@@ -726,7 +727,7 @@ static int load_flat_file(struct linux_b
for (rp = (unsigned long *)datapos; *rp != 0xffffffff; rp++) {
unsigned long addr;
if (*rp) {
- addr = calc_reloc(*rp, libinfo, id, 0);
+ addr = calc_reloc(bprm, *rp, libinfo, id, 0);
if (addr == RELOC_FAILED) {
ret = -ENOEXEC;
goto err;
@@ -759,7 +760,7 @@ static int load_flat_file(struct linux_b
if (flat_set_persistent (relval, &persistent))
continue;
addr = flat_get_relocate_addr(relval);
- rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
+ rp = (unsigned long *) calc_reloc(bprm, addr, libinfo, id, 1);
if (rp == (unsigned long *)RELOC_FAILED) {
ret = -ENOEXEC;
goto err;
@@ -775,7 +776,7 @@ static int load_flat_file(struct linux_b
*/
if ((flags & FLAT_FLAG_GOTPIC) == 0)
addr = ntohl(addr);
- addr = calc_reloc(addr, libinfo, id, 0);
+ addr = calc_reloc(bprm, addr, libinfo, id, 0);
if (addr == RELOC_FAILED) {
ret = -ENOEXEC;
goto err;
@@ -812,37 +813,29 @@ err:
* segment (including bss) but not argv/argc/environ.
*/
-static int load_flat_shared_library(int id, struct lib_info *libs)
+static int load_flat_shared_library(struct linux_binprm *bprm, int id,
+ struct lib_info *libs)
{
- struct linux_binprm bprm;
- int res;
+ char *save_name = bprm->filename;
+ struct file *save_file = bprm->file;
char buf[16];
-
- /* Create the file name */
- sprintf(buf, "/lib/lib%d.so", id);
+ int res;
/* Open the file up */
- bprm.filename = buf;
- bprm.file = open_exec(bprm.filename);
- res = PTR_ERR(bprm.file);
- if (IS_ERR(bprm.file))
- return res;
-
- bprm.cred = prepare_exec_creds();
- res = -ENOMEM;
- if (!bprm.cred)
- goto out;
-
- res = prepare_binprm(&bprm);
-
- if (res <= (unsigned long)-4096)
- res = load_flat_file(&bprm, libs, id, NULL);
-
- abort_creds(bprm.cred);
-
-out:
- allow_write_access(bprm.file);
- fput(bprm.file);
+ sprintf(buf, "/lib/lib%d.so", id);
+ bprm->filename = buf;
+ bprm->file = open_exec(bprm->filename);
+ res = PTR_ERR(bprm->file);
+ if (IS_ERR(bprm->file))
+ goto ret;
+
+ res = load_flat_file(bprm, libs, id, NULL);
+
+ allow_write_access(bprm->file);
+ fput(bprm->file);
+ret:
+ bprm->filename = save_name;
+ bprm->file = save_file;
return(res);
}
[Oleg Nesterov - Thu, Sep 03, 2009 at 06:29:39PM +0200]
| On 09/03, Oleg Nesterov wrote:
| >
| > load_flat_shared_library() does something strange (but hopefully this
| > patch doesn't break it). I do not understand why does it create the
| > new bprm. Afaics, it could reuse bprm pointer which comes as an argument
| > of ->load_binary(), all we need is to temporary change/restore bprm->file
| > for load_flat_file().
|
| IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
| plays with prepare_exec_creds().
|
| But again, I don't understand this code, and I didn't even try to compile
| this patch.
|
| Oleg.
|
...
| -static int load_flat_shared_library(int id, struct lib_info *libs)
| +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
| + struct lib_info *libs)
| {
...
| + sprintf(buf, "/lib/lib%d.so", id);
Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
as well? buf is only 16 bytes long so we have 4 byte room for number.
Not sure if it's possible to have 10000 relocs though :) Just a thought.
Most probably I miss something.
-- Cyrill
[Cyrill Gorcunov - Thu, Sep 03, 2009 at 08:58:50PM +0400]
...
| ...
| | -static int load_flat_shared_library(int id, struct lib_info *libs)
| | +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
| | + struct lib_info *libs)
| | {
| ...
| | + sprintf(buf, "/lib/lib%d.so", id);
|
| Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
| as well? buf is only 16 bytes long so we have 4 byte room for number.
| Not sure if it's possible to have 10000 relocs though :) Just a thought.
| Most probably I miss something.
|
| -- Cyrill
Sigh... I'm idiot. We have MAX_SHARED_LIBS limit here. Drop my question
please. Sorry for noise.
-- Cyrill
On 09/03, Cyrill Gorcunov wrote:
>
> [Oleg Nesterov - Thu, Sep 03, 2009 at 06:29:39PM +0200]
> | On 09/03, Oleg Nesterov wrote:
> | >
> | > load_flat_shared_library() does something strange (but hopefully this
> | > patch doesn't break it). I do not understand why does it create the
> | > new bprm. Afaics, it could reuse bprm pointer which comes as an argument
> | > of ->load_binary(), all we need is to temporary change/restore bprm->file
> | > for load_flat_file().
> |
> | IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
> | plays with prepare_exec_creds().
> |
> | But again, I don't understand this code, and I didn't even try to compile
> | this patch.
> |
> | Oleg.
> |
> ...
> | -static int load_flat_shared_library(int id, struct lib_info *libs)
> | +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
> | + struct lib_info *libs)
> | {
> ...
> | + sprintf(buf, "/lib/lib%d.so", id);
>
> Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
> as well? buf is only 16 bytes long so we have 4 byte room for number.
Agreed. As you pointed out privately we have MAX_SHARED_LIBS=4, but
still snprintf() is safer.
Oleg.
Oleg Nesterov <[email protected]> wrote:
> [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex
I don't see where it is sleeping in TASK_TRACED with cred_guard_mutex held.
David
> Oleg Nesterov <[email protected]> wrote:
>
> > [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex
>
> I don't see where it is sleeping in TASK_TRACED with cred_guard_mutex held.
search_binary_handler calls tracehook_report_exec, which calls
ptrace_notify when PTRACE_O_TRACEEXEC is enabled.
Thanks,
Roland
Roland McGrath <[email protected]> wrote:
> search_binary_handler calls tracehook_report_exec, which calls
> ptrace_notify when PTRACE_O_TRACEEXEC is enabled.
Ah. Now I see.
David