[PATCH] binfmt_flat: fix sparse annotation ordering

[PATCH] binfmt_flat: fix sparse annotation ordering

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The sparse annotation ordering inside the function call is swapped.

Fix the ordering so that we silence the following sparse warnings:
fs/binfmt_flat.c:816:39: warning: incorrect type in argument 1 (different address spaces)
fs/binfmt_flat.c:816:39:    expected unsigned int [noderef] [usertype] __user *rp
fs/binfmt_flat.c:816:39:    got unsigned int [usertype] *[noderef] __user

No functional change as sparse annotations are ignored by the compiler.

Fixes: a767e6fd68d2 ("binfmt_flat: do not stop relocating GOT entries prematurely on riscv")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Cc: <stable@vger.kernel.org>
---
Hello Kees,

Sorry about this.
Feel free to squash it with the existing patch if you so like.

 fs/binfmt_flat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index e5e2a03b39c1..dca0b6875f9c 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -813,7 +813,7 @@ static int load_flat_file(struct linux_binprm *bprm,
 	 * image.
 	 */
 	if (flags & FLAT_FLAG_GOTPIC) {
-		rp = skip_got_header((u32 * __user) datapos);
+		rp = skip_got_header((u32 __user *) datapos);
 		for (; ; rp++) {
 			u32 addr, rp_val;
 			if (get_user(rp_val, rp))
-- 
2.35.1

Re: [PATCH] binfmt_flat: fix sparse annotation ordering

[Kees Cook]

On Mon, Apr 18, 2022 at 08:09:12PM +0000, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The sparse annotation ordering inside the function call is swapped.
> 
> Fix the ordering so that we silence the following sparse warnings:
> fs/binfmt_flat.c:816:39: warning: incorrect type in argument 1 (different address spaces)
> fs/binfmt_flat.c:816:39:    expected unsigned int [noderef] [usertype] __user *rp
> fs/binfmt_flat.c:816:39:    got unsigned int [usertype] *[noderef] __user
> 
> No functional change as sparse annotations are ignored by the compiler.
> 
> Fixes: a767e6fd68d2 ("binfmt_flat: do not stop relocating GOT entries prematurely on riscv")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> Cc: <stable@vger.kernel.org>
> ---
> Hello Kees,
> 
> Sorry about this.
> Feel free to squash it with the existing patch if you so like.

Ah-ha! Thanks; I was just looking at the sparse email. :)

-Kees

> 
>  fs/binfmt_flat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index e5e2a03b39c1..dca0b6875f9c 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -813,7 +813,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>  	 * image.
>  	 */
>  	if (flags & FLAT_FLAG_GOTPIC) {
> -		rp = skip_got_header((u32 * __user) datapos);
> +		rp = skip_got_header((u32 __user *) datapos);
>  		for (; ; rp++) {
>  			u32 addr, rp_val;
>  			if (get_user(rp_val, rp))
> -- 
> 2.35.1

-- 
Kees Cook

[PATCH] binfmt_flat; Drop vestigates of coredump support

[Eric W. Biederman]

There is the briefest start of coredump support in binfmt_flat.  It is
actually a pain to maintain as binfmt_flat is not built on most
architectures so it is easy to overlook.

Since the support does not do anything remove it.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

Apologies for hijacking this thread but it looks like we have people who
are actively using binfmt_flat on it.

Does anyone have any objections to simply removing what little there
is of coredump support from binfmt_flat?

Eric

 fs/binfmt_flat.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 626898150011..0ad2c7bbaddd 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -37,7 +37,6 @@
 #include <linux/flat.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
-#include <linux/coredump.h>
 
 #include <asm/byteorder.h>
 #include <asm/unaligned.h>
@@ -98,33 +97,12 @@ static int load_flat_shared_library(int id, struct lib_info *p);
 #endif
 
 static int load_flat_binary(struct linux_binprm *);
-#ifdef CONFIG_COREDUMP
-static int flat_core_dump(struct coredump_params *cprm);
-#endif
 
 static struct linux_binfmt flat_format = {
 	.module		= THIS_MODULE,
 	.load_binary	= load_flat_binary,
-#ifdef CONFIG_COREDUMP
-	.core_dump	= flat_core_dump,
-	.min_coredump	= PAGE_SIZE
-#endif
 };
 
-/****************************************************************************/
-/*
- * Routine writes a core dump image in the current directory.
- * Currently only a stub-function.
- */
-
-#ifdef CONFIG_COREDUMP
-static int flat_core_dump(struct coredump_params *cprm)
-{
-	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
-		current->comm, current->pid, cprm->siginfo->si_signo);
-	return 1;
-}
-#endif
 
 /****************************************************************************/
 /*
-- 
2.35.3

Re: [PATCH] binfmt_flat; Drop vestigates of coredump support

[Niklas Cassel]

On Tue, Apr 19, 2022 at 09:16:41AM -0500, Eric W. Biederman wrote:
> 
> There is the briefest start of coredump support in binfmt_flat.  It is
> actually a pain to maintain as binfmt_flat is not built on most
> architectures so it is easy to overlook.
> 
> Since the support does not do anything remove it.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> 
> Apologies for hijacking this thread but it looks like we have people who
> are actively using binfmt_flat on it.
> 
> Does anyone have any objections to simply removing what little there
> is of coredump support from binfmt_flat?
> 
> Eric
> 
>  fs/binfmt_flat.c | 22 ----------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 626898150011..0ad2c7bbaddd 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -37,7 +37,6 @@
>  #include <linux/flat.h>
>  #include <linux/uaccess.h>
>  #include <linux/vmalloc.h>
> -#include <linux/coredump.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/unaligned.h>
> @@ -98,33 +97,12 @@ static int load_flat_shared_library(int id, struct lib_info *p);
>  #endif
>  
>  static int load_flat_binary(struct linux_binprm *);
> -#ifdef CONFIG_COREDUMP
> -static int flat_core_dump(struct coredump_params *cprm);
> -#endif
>  
>  static struct linux_binfmt flat_format = {
>  	.module		= THIS_MODULE,
>  	.load_binary	= load_flat_binary,
> -#ifdef CONFIG_COREDUMP
> -	.core_dump	= flat_core_dump,
> -	.min_coredump	= PAGE_SIZE
> -#endif
>  };
>  
> -/****************************************************************************/
> -/*
> - * Routine writes a core dump image in the current directory.
> - * Currently only a stub-function.
> - */
> -
> -#ifdef CONFIG_COREDUMP
> -static int flat_core_dump(struct coredump_params *cprm)
> -{
> -	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
> -		current->comm, current->pid, cprm->siginfo->si_signo);
> -	return 1;
> -}
> -#endif

Since this only prints a warning that the process "should have core dumped",
I agree, I don't really see the point of keeping this code.

nit: $subject: "binfmt_flat; Drop vestigates of coredump support"
s/;/:/

Other than that:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: (subset) [PATCH] binfmt_flat; Drop vestigates of coredump support

[Kees Cook]

On Tue, 19 Apr 2022 09:16:41 -0500, Eric W. Biederman wrote:
> There is the briefest start of coredump support in binfmt_flat.  It is
> actually a pain to maintain as binfmt_flat is not built on most
> architectures so it is easy to overlook.
> 
> Since the support does not do anything remove it.
> 
> 
> [...]

Applied to for-next/execve, thanks! (With typo nits fixed.)

[1/1] binfmt_flat; Drop vestigates of coredump support
      https://git.kernel.org/kees/c/6e1a873cefd1

-- 
Kees Cook

Re: (subset) [PATCH] binfmt_flat; Drop vestigates of coredump support

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Tue, 19 Apr 2022 09:16:41 -0500, Eric W. Biederman wrote:
>> There is the briefest start of coredump support in binfmt_flat.  It is
>> actually a pain to maintain as binfmt_flat is not built on most
>> architectures so it is easy to overlook.
>> 
>> Since the support does not do anything remove it.
>> 
>> 
>> [...]
>
> Applied to for-next/execve, thanks! (With typo nits fixed.)
>
> [1/1] binfmt_flat; Drop vestigates of coredump support
>       https://git.kernel.org/kees/c/6e1a873cefd1

Thanks,
Eric

Re: [PATCH] binfmt_flat; Drop vestigates of coredump support

[Greg Ungerer]

On 20/4/22 00:16, Eric W. Biederman wrote:
> There is the briefest start of coredump support in binfmt_flat.  It is
> actually a pain to maintain as binfmt_flat is not built on most
> architectures so it is easy to overlook.
> 
> Since the support does not do anything remove it.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> 
> Apologies for hijacking this thread but it looks like we have people who
> are actively using binfmt_flat on it.
> 
> Does anyone have any objections to simply removing what little there
> is of coredump support from binfmt_flat?

No objections from me.

Acked-by: Greg Ungerer <gerg@linux-m68k.org>


> Eric
> 
>   fs/binfmt_flat.c | 22 ----------------------
>   1 file changed, 22 deletions(-)
> 
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 626898150011..0ad2c7bbaddd 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -37,7 +37,6 @@
>   #include <linux/flat.h>
>   #include <linux/uaccess.h>
>   #include <linux/vmalloc.h>
> -#include <linux/coredump.h>
>   
>   #include <asm/byteorder.h>
>   #include <asm/unaligned.h>
> @@ -98,33 +97,12 @@ static int load_flat_shared_library(int id, struct lib_info *p);
>   #endif
>   
>   static int load_flat_binary(struct linux_binprm *);
> -#ifdef CONFIG_COREDUMP
> -static int flat_core_dump(struct coredump_params *cprm);
> -#endif
>   
>   static struct linux_binfmt flat_format = {
>   	.module		= THIS_MODULE,
>   	.load_binary	= load_flat_binary,
> -#ifdef CONFIG_COREDUMP
> -	.core_dump	= flat_core_dump,
> -	.min_coredump	= PAGE_SIZE
> -#endif
>   };
>   
> -/****************************************************************************/
> -/*
> - * Routine writes a core dump image in the current directory.
> - * Currently only a stub-function.
> - */
> -
> -#ifdef CONFIG_COREDUMP
> -static int flat_core_dump(struct coredump_params *cprm)
> -{
> -	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
> -		current->comm, current->pid, cprm->siginfo->si_signo);
> -	return 1;
> -}
> -#endif
>   
>   /****************************************************************************/
>   /*