RE: [PATCH] Handle non-readable binfmt misc executables

RE: [PATCH] Handle non-readable binfmt misc executables

[Albert Cahalan]

On Sun, 2004-06-20 at 06:36, Zach, Yoav wrote:
> Albert,
> I'm a little bit confused - I see myself in the CC: list, with no
> recipient in the To: list. I'm not sure this reply will get to all
> the recipients of your message. Will you please forward it to all
> the rest ?

I don't know if I did send it elsewhere. Maybe I had
intended to add linux-kernel.

> Anyways, the problem with the info in /proc can be solved by
> the translator itself in userland. The way it is done in the IA-32
> Execution Layer is by shifting the arguments vector two positions to
> the left, so the user can see the original argument list without the
> additional information the kernel uses for the binfmt_misc mechanism.

So the content of /proc/*/cmdline is correct?

At a minimum, you will have a problem at startup.
The process might be observed before you fix argv.

What about apps that walk off the end of argv to get
at the environment?

It seems cleaner to use some other mechanism.
Assuming your interpreter is ELF, ELF notes are good.
You might use prctl().

RE: [PATCH] Handle non-readable binfmt misc executables

[Zach, Yoav]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

 

>-----Original Message-----
>From: Albert Cahalan [mailto:albert@users.sourceforge.net] 
>Sent: Monday, June 21, 2004 14:50
>To: Zach, Yoav
>Cc: Albert Cahalan; linux-kernel mailing list
>Subject: RE: [PATCH] Handle non-readable binfmt misc executables
>

>> Right. It might happen once in a (long) while that
>> 'ps -f' doesn't show the correct command line. 
>
>So this is a hole in the emulation.
>

You have a point here. The patch below fixes this problem
by passing the open fd via the aux-vector. Argv[1] will
be the full path to the binary regardless of whether the
kernel opened it or not. The patch is against recent mm tree,
thus it needs to be applied on top of two prev patches to
binfmt_misc that are part of this tree.
As my mailer tends to wrap up text, the patch is also attached.


Thanks,
Yoav.



========================= BEGIN PATCH =========================
diff -r -U 3 -p linux-2.6/fs/binfmt_elf.c linux/fs/binfmt_elf.c
--- linux-2.6/fs/binfmt_elf.c	2004-06-23 08:13:28.000000000 +0300
+++ linux/fs/binfmt_elf.c	2004-06-23 12:33:51.075747675 +0300
@@ -202,6 +202,9 @@ create_elf_tables(struct linux_binprm *b
 	if (k_platform) {
 		NEW_AUX_ENT(AT_PLATFORM, (elf_addr_t)(long)u_platform);
 	}
+	if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) {
+		NEW_AUX_ENT(AT_EXECFD, (elf_addr_t) BINPRM_GET_EXECFD
(bprm->interp_flags));
+	}
 #undef NEW_AUX_ENT
 	/* AT_NULL is zero; clear the rest too */
 	memset(&elf_info[ei_index], 0,
diff -r -U 3 -p linux-2.6/fs/binfmt_misc.c linux/fs/binfmt_misc.c
--- linux-2.6/fs/binfmt_misc.c	2004-06-23 08:13:26.000000000 +0300
+++ linux/fs/binfmt_misc.c	2004-06-23 12:33:51.081607003 +0300
@@ -109,7 +109,6 @@ static int load_misc_binary(struct linux
 	char *iname_addr = iname;
 	int retval;
 	int fd_binary = -1;
-	char fd_str[12];
 	struct files_struct *files = NULL;
 
 	retval = -ENOEXEC;
@@ -130,7 +129,6 @@ static int load_misc_binary(struct linux
 	}
 
 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
-		char *fdsp = fd_str;
 
 		files = current->files;
 		retval = unshare_files();
@@ -158,27 +156,27 @@ static int load_misc_binary(struct linux
 		allow_write_access(bprm->file);
 		bprm->file = NULL;
 
-		/* make argv[1] be the file descriptor of the binary */
- 		snprintf(fd_str, sizeof(fd_str), "%d", fd_binary);
- 		retval = copy_strings_kernel(1, &fdsp, bprm);
-		if (retval < 0)
-			goto _error;
-		bprm->argc++;
+		/* mark the bprm that fd should be passed to interp */
+		bprm->interp_flags |= (BINPRM_FLAGS_EXECFD | 
+					BINPRM_SET_EXECFD (fd_binary));
 
  	} else {
  		allow_write_access(bprm->file);
  		fput(bprm->file);
  		bprm->file = NULL;
-		/* make argv[1] be the path to the binary */
- 		retval = copy_strings_kernel (1, &bprm->interp, bprm);
-		if (retval < 0)
-			goto _error;
-		bprm->argc++;
  	}
+	/* make argv[1] be the path to the binary */
+	retval = copy_strings_kernel (1, &bprm->interp, bprm);
+	if (retval < 0)
+		goto _error;
+	bprm->argc++;
+
+	/* add the interp as argv[0] */
 	retval = copy_strings_kernel (1, &iname_addr, bprm);
 	if (retval < 0)
 		goto _error;
 	bprm->argc ++;
+
 	bprm->interp = iname;	/* for binfmt_script */
 
 	interp_file = open_exec (iname);
diff -r -U 3 -p linux-2.6/include/linux/binfmts.h
linux/include/linux/binfmts.h
--- linux-2.6/include/linux/binfmts.h	2004-06-23 08:14:39.000000000
+0300
+++ linux/include/linux/binfmts.h	2004-06-23 12:33:51.082583557
+0300
@@ -42,6 +42,14 @@ struct linux_binprm{
 #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
 #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 <<
BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
 
+/* fd of the binary should be passed to the interpreter */
+#define BINPRM_FLAGS_EXECFD_BIT 1
+#define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
+/* the fd is kept in the high 32 bit of the flags */
+#define BINPRM_GET_EXECFD(flg) (((flg) >> 32) & 0xfffffffful)
+#define BINPRM_SET_EXECFD(fd) (((unsigned long)(fd) << 32) &
0xffffffff00000000ul)
+
+
 /*
  * This structure defines the functions that are used to load the
binary formats that
  * linux accepts.
========================= END   PATCH =========================

[-- Attachment #2: binfmt_misc-nonreadable-execfd.patch --]
[-- Type: application/octet-stream, Size: 3153 bytes --]

diff -r -U 3 -p linux-2.6/fs/binfmt_elf.c linux/fs/binfmt_elf.c
--- linux-2.6/fs/binfmt_elf.c	2004-06-23 08:13:28.000000000 +0300
+++ linux/fs/binfmt_elf.c	2004-06-23 12:33:51.075747675 +0300
@@ -202,6 +202,9 @@ create_elf_tables(struct linux_binprm *b
 	if (k_platform) {
 		NEW_AUX_ENT(AT_PLATFORM, (elf_addr_t)(long)u_platform);
 	}
+	if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) {
+		NEW_AUX_ENT(AT_EXECFD, (elf_addr_t) BINPRM_GET_EXECFD (bprm->interp_flags));
+	}
 #undef NEW_AUX_ENT
 	/* AT_NULL is zero; clear the rest too */
 	memset(&elf_info[ei_index], 0,
diff -r -U 3 -p linux-2.6/fs/binfmt_misc.c linux/fs/binfmt_misc.c
--- linux-2.6/fs/binfmt_misc.c	2004-06-23 08:13:26.000000000 +0300
+++ linux/fs/binfmt_misc.c	2004-06-23 12:33:51.081607003 +0300
@@ -109,7 +109,6 @@ static int load_misc_binary(struct linux
 	char *iname_addr = iname;
 	int retval;
 	int fd_binary = -1;
-	char fd_str[12];
 	struct files_struct *files = NULL;
 
 	retval = -ENOEXEC;
@@ -130,7 +129,6 @@ static int load_misc_binary(struct linux
 	}
 
 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
-		char *fdsp = fd_str;
 
 		files = current->files;
 		retval = unshare_files();
@@ -158,27 +156,27 @@ static int load_misc_binary(struct linux
 		allow_write_access(bprm->file);
 		bprm->file = NULL;
 
-		/* make argv[1] be the file descriptor of the binary */
- 		snprintf(fd_str, sizeof(fd_str), "%d", fd_binary);
- 		retval = copy_strings_kernel(1, &fdsp, bprm);
-		if (retval < 0)
-			goto _error;
-		bprm->argc++;
+		/* mark the bprm that fd should be passed to interp */
+		bprm->interp_flags |= (BINPRM_FLAGS_EXECFD | 
+					BINPRM_SET_EXECFD (fd_binary));
 
  	} else {
  		allow_write_access(bprm->file);
  		fput(bprm->file);
  		bprm->file = NULL;
-		/* make argv[1] be the path to the binary */
- 		retval = copy_strings_kernel (1, &bprm->interp, bprm);
-		if (retval < 0)
-			goto _error;
-		bprm->argc++;
  	}
+	/* make argv[1] be the path to the binary */
+	retval = copy_strings_kernel (1, &bprm->interp, bprm);
+	if (retval < 0)
+		goto _error;
+	bprm->argc++;
+
+	/* add the interp as argv[0] */
 	retval = copy_strings_kernel (1, &iname_addr, bprm);
 	if (retval < 0)
 		goto _error;
 	bprm->argc ++;
+
 	bprm->interp = iname;	/* for binfmt_script */
 
 	interp_file = open_exec (iname);
diff -r -U 3 -p linux-2.6/include/linux/binfmts.h linux/include/linux/binfmts.h
--- linux-2.6/include/linux/binfmts.h	2004-06-23 08:14:39.000000000 +0300
+++ linux/include/linux/binfmts.h	2004-06-23 12:33:51.082583557 +0300
@@ -42,6 +42,14 @@ struct linux_binprm{
 #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
 #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
 
+/* fd of the binary should be passed to the interpreter */
+#define BINPRM_FLAGS_EXECFD_BIT 1
+#define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
+/* the fd is kept in the high 32 bit of the flags */
+#define BINPRM_GET_EXECFD(flg) (((flg) >> 32) & 0xfffffffful)
+#define BINPRM_SET_EXECFD(fd) (((unsigned long)(fd) << 32) & 0xffffffff00000000ul)
+
+
 /*
  * This structure defines the functions that are used to load the binary formats that
  * linux accepts.

RE: [PATCH] Handle non-readable binfmt_misc executables

[Zach, Yoav]

 

>-----Original Message-----
>From: Arjan van de Ven [mailto:arjanv@redhat.com] 
>Sent: Sunday, June 20, 2004 13:50
>To: Zach, Yoav
>Cc: Linux Kernel Mailing List
>Subject: Re: [PATCH] Handle non-readable binfmt_misc executables
>

>
>for one, sys_close, while currently exported, shouldn't be really.
>(it is exported right now for a few drivers that have invalid firmware
>loaders that haven't been converted to the firmware loading framework).

What is the reason that sys_close should not be used by modules ?

>In addition it's way overkill, you created the fd so half the safety
>precautions shouldn/t be needed
>

There are some checks that might be skipped. But I think that calling
sys_close is the right thing to do here, because this way future changes
to the procedure of closing an open fd will not need to be copied to
the recovery code of load_misc_binary. Do you expect there will be any
visible gain in performance if the unnecessary steps are skipped ?
Please
remember this is only recovery code - this is not the main stream
execution.

Thanks,
Yoav.

Yoav Zach
IA-32 Execution Layer
Performance Tools Lab
Intel Corp.

RE: [PATCH] Handle non-readable binfmt misc executables

[Zach, Yoav]

>-----Original Message-----
>From: Albert Cahalan [mailto:albert@users.sourceforge.net] 
>Sent: Sunday, June 20, 2004 15:17
>To: Zach, Yoav
>Cc: linux-kernel mailing list
>Subject: RE: [PATCH] Handle non-readable binfmt misc executables
>


>So the content of /proc/*/cmdline is correct?
>

After the translator fixes it - yes.

>At a minimum, you will have a problem at startup.
>The process might be observed before you fix argv.
>

Right. It might happen once in a (long) while that
'ps -f' doesn't show the correct command line. 

>What about apps that walk off the end of argv to get
>at the environment?
>

Please note that the stack is that of the translator, which
is aware of the fixing of argv.

>It seems cleaner to use some other mechanism.
>Assuming your interpreter is ELF, ELF notes are good.

Using ELF notes means changing the binaries, which is not
suitable for cases where the use of translator for running
the binaries is not 'known' to the binaries. For example,
an administrator might start using a translator to enhance
performance of existing binaries. In such a case, re-building
the binaries will probably be out of the question.

>You might use prctl().
>

Do you mean enhancing sys_prctl to allow for fixing 
the argv ? 

Thanks,
Yoav.

RE: [PATCH] Handle non-readable binfmt misc executables

[Albert Cahalan]

On Mon, 2004-06-21 at 07:31, Zach, Yoav wrote:
> >From: Albert Cahalan [mailto:albert@users.sourceforge.net] 

> >So the content of /proc/*/cmdline is correct?
> 
> After the translator fixes it - yes.
> 
> >At a minimum, you will have a problem at startup.
> >The process might be observed before you fix argv.
> 
> Right. It might happen once in a (long) while that
> 'ps -f' doesn't show the correct command line. 

So this is a hole in the emulation.

> >It seems cleaner to use some other mechanism.
> >Assuming your interpreter is ELF, ELF notes are good.
> 
> Using ELF notes means changing the binaries, which is not
> suitable for cases where the use of translator for running
> the binaries is not 'known' to the binaries. For example,
> an administrator might start using a translator to enhance
> performance of existing binaries. In such a case, re-building
> the binaries will probably be out of the question.

No. Well, the translator would change. The old i386
binaries would not.

ELF notes are supplied by the kernel. They provide
data like USER_HZ, the UID, a flag to indicate that
ld.so must take setuid-type precautions, and so on.
ELF notes are on the stack, beyond the environment.

> >You might use prctl().
> 
> Do you mean enhancing sys_prctl to allow for fixing 
> the argv ? 

No. I mean enhancing sys_prctl to allow asking for
the file descriptor number. That way, argv doesn't
need to get mangled in the first place.

I'm sure there are many other good ways to pass the
file descriptor number to the interpreter.

RE: [PATCH] Handle non-readable binfmt_misc executables

[Zach, Yoav]

I'm not sure I understand the problem. Load_elf_binary also
uses sys_close for recovery in case of error. Can you please
give me more details on the problems you see with using sys_close ?

Thanks,
Yoav.

Yoav Zach
IA-32 Execution Layer
Performance Tools Lab
Intel Corp.


>-----Original Message-----
>From: Arjan van de Ven [mailto:arjanv@redhat.com] 
>Sent: Saturday, June 19, 2004 17:39
>To: Linux Kernel Mailing List
>Cc: Zach, Yoav; torvalds@osdl.org
>Subject: Re: [PATCH] Handle non-readable binfmt_misc executables
>
>
>> +_error_close_file:
>> +	if (fd_binary > 0) {
>> +		sys_close (fd_binary);
>> +		fd_binary = -1;
>> +		bprm->file = NULL;
>> +	
>
>
>ewww sys_close.... there HAS to be a better way to do that... 
>

Re: [PATCH] Handle non-readable binfmt_misc executables

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Sun, Jun 20, 2004 at 01:41:30PM +0300, Zach, Yoav wrote:
> I'm not sure I understand the problem. Load_elf_binary also
> uses sys_close for recovery in case of error. Can you please
> give me more details on the problems you see with using sys_close ?

for one, sys_close, while currently exported, shouldn't be really.
(it is exported right now for a few drivers that have invalid firmware
loaders that haven't been converted to the firmware loading framework).
In addition it's way overkill, you created the fd so half the safety
precautions shouldn/t be needed

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Handle non-readable binfmt_misc executables

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]


> +_error_close_file:
> +	if (fd_binary > 0) {
> +		sys_close (fd_binary);
> +		fd_binary = -1;
> +		bprm->file = NULL;
> +	


ewww sys_close.... there HAS to be a better way to do that... 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]