[PATCH] Kill a sparse warning in binfmt_elf.c

[PATCH] Kill a sparse warning in binfmt_elf.c

[Arun Sharma]

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


The attached patch kills a sparse warning in binfmt_elf.c:dump_write() by adding a __user annotation.

	-Arun


[-- Attachment #2: sparse-binfmt.patch --]
[-- Type: text/plain, Size: 539 bytes --]

Kill a sparse warning in 2.6.9-rc3.

Signed-off-by: Arun Sharma <arun.sharma@intel.com>

===== fs/binfmt_elf.c 1.88 vs edited =====
--- 1.88/fs/binfmt_elf.c	Wed Sep 22 13:34:05 2004
+++ edited/fs/binfmt_elf.c	Wed Oct  6 13:16:37 2004
@@ -1032,7 +1032,7 @@
  */
 static int dump_write(struct file *file, const void *addr, int nr)
 {
-	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
+	return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
 }
 
 static int dump_seek(struct file *file, off_t off)

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Valdis.Kletnieks]

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

On Wed, 06 Oct 2004 15:45:02 PDT, Arun Sharma said:

> The attached patch kills a sparse warning in binfmt_elf.c:dump_write() by
> adding a __user annotation.

>  static int dump_write(struct file *file, const void *addr, int nr)
>  {
> -	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> +	return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
>  }

wouldn't it be more useful to put the annotation into the *prototype* for
the dump_write() function, so that we get sparse typechecking for the
caller(s) of this function?  Your fix just kills the warning - when the *real*
problem is that we're called with a 'void *' that we then cast to something
without any real double check on what it is....

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

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Arun Sharma]

On 10/7/2004 11:11 AM, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 06 Oct 2004 15:45:02 PDT, Arun Sharma said:
> 
>> The attached patch kills a sparse warning in binfmt_elf.c:dump_write() by
>> adding a __user annotation.
> 
>>  static int dump_write(struct file *file, const void *addr, int nr)
>>  {
>> -	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
>> +	return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
>>  }
> 
> wouldn't it be more useful to put the annotation into the *prototype* for
> the dump_write() function, so that we get sparse typechecking for the
> caller(s) of this function?  Your fix just kills the warning - when the *real*
> problem is that we're called with a 'void *' that we then cast to something
> without any real double check on what it is....

dump_write() is a static function without a prototype.

	-Arun

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Valdis.Kletnieks]

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

On Thu, 07 Oct 2004 11:49:24 PDT, Arun Sharma said:

> >>  static int dump_write(struct file *file, const void *addr, int nr)
> >>  {
> >> -	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> >> +	return file->f_op->write(file, (const char __user *) addr, nr, &file->f
_pos) == nr;
> >>  }
> > 
> > wouldn't it be more useful to put the annotation into the *prototype* for
> > the dump_write() function, so that we get sparse typechecking for the
> > caller(s) of this function?  Your fix just kills the warning - when the *re
al*
> > problem is that we're called with a 'void *' that we then cast to something
> > without any real double check on what it is....
> 
> dump_write() is a static function without a prototype.

static int dump_write(struct file *file, const char __user *addr, int nr)
{
	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
}

And then go find the callers and make sure what they're passing is a
'const char __user *' rather than a 'const void *'....

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

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Arun Sharma]

On 10/7/2004 11:54 AM, Valdis.Kletnieks@vt.edu wrote:

> And then go find the callers and make sure what they're passing is a
> 'const char __user *' rather than a 'const void *'....

Many callers in binfmt_elf.c are passing pointers that are kernel addresses. But file_operations->write() is expecting a __user pointer. The intention of the cast is to explicitly say that's an okay thing to do.

	-Arun

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[David S. Miller]

On Thu, 07 Oct 2004 14:54:57 -0400
Valdis.Kletnieks@vt.edu wrote:

> > dump_write() is a static function without a prototype.
> 
> static int dump_write(struct file *file, const char __user *addr, int nr)
> {
> 	return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> }
> 
> And then go find the callers and make sure what they're passing is a
> 'const char __user *' rather than a 'const void *'....

The caller's aren't, that is the point.  They run dump_write()
with set_fs(KERNEL_DS), which allows kernel pointers to be treated
as user ones in system call handling paths, which is why the cast
is needed somewhere.

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Valdis.Kletnieks]

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

On Thu, 07 Oct 2004 12:01:19 PDT, Arun Sharma said:

> Many callers in binfmt_elf.c are passing pointers that are kernel addresses.
> But file_operations->write() is expecting a __user pointer. The intention of
> the cast is to explicitly say that's an okay thing to do.

Right.  My point was that if you're fixing the warning, you should do the
*whole* job by making sure that what *you* got in that 'void *' is a __user
pointer, rather than just saying "we know write() wants a __user".

So for instance, up a level in writenote():

#define DUMP_WRITE(addr, nr)            do { if (!dump_write(file, (addr), (nr))) return 0; } while(0)
#define DUMP_SEEK(off)          do { if (!dump_seek(file, (off))) return 0; } while(0)

static int writenote(struct memelfnote *men, struct file *file)
{
        struct elf_note en;

        en.n_namesz = strlen(men->name) + 1;
        en.n_descsz = men->datasz;
        en.n_type = men->type;

        DUMP_WRITE(&en, sizeof(en));
        DUMP_WRITE(men->name, en.n_namesz);

Is men->name a __user pointer?  Should it be? Can it be something else?

struct memelfnote
{
        const char *name;
        int type;
        unsigned int datasz;
        void *data;
};

Hmm.. there it's a 'const char *name', but we feed it to a 'const void *'
and then you cast it to a __user.  Either that *name should be __user
as well, or you tagged something as a __user that might not be.

*That*s what I'm talking about....


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

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Valdis.Kletnieks]

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

On Thu, 07 Oct 2004 12:16:23 PDT, "David S. Miller" said:

> The caller's aren't, that is the point.  They run dump_write()
> with set_fs(KERNEL_DS), which allows kernel pointers to be treated
> as user ones in system call handling paths, which is why the cast
> is needed somewhere.

Right - and my point is that putting one cast way down at the bottom
to quiet a warning doesn't do much good - the cast should be pushed
up to as close to that set_fs() as feasible.  Otherwise if some other,
new, caller surfaces and bogusly passes something *else* in that
void * pointer, it gets a lot harder for sparse and similar to do their
jobs. 


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

Re: [PATCH] Kill a sparse warning in binfmt_elf.c

[Arun Sharma]

On 10/7/2004 12:16 PM, Valdis.Kletnieks@vt.edu wrote:

> Hmm.. there it's a 'const char *name', but we feed it to a 'const void *'
> and then you cast it to a __user. 

I'm not very concerned about const char * being cast to const void *. That's not the focus of the patch[1]. The focus of the patch is really __user.

> Either that *name should be __user
> as well, or you tagged something as a __user that might not be.
>

Certain interfaces such as get_user() will fail if you pass a kernel pointer and unconditionally cast it to __user (without doing a set_fs(KERNEL_DS)). But file_operations->write() is not one of them and hence the patch.

	-Arun

[1] If you insist on this, you should change all the calls to memcpy() as well.