Kill a sparse warning in binfmt_elf.c

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

-Arun

Kill a sparse warning in 2.6.9-rc3.

===== fs/binfmt_elf.c 1.88 vs edited =====

@@ -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)

dump_write() is a static function without a prototype.

-Arun

_pos) == nr;

al*

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 *'....

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

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....

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.

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.

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.