linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [CHECKER][PATCH] awe_wave.c user pointer dereference
@ 2003-06-05 21:34 Hollis Blanchard
  2003-06-05 22:07 ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Hollis Blanchard @ 2003-06-05 21:34 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel

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

Two ioctl functions in sound/oss/awe_wave.c were directly dereferencing 
a user-supplied pointer in a few places. Please apply.

-- 
Hollis Blanchard
IBM Linux Technology Center


[-- Attachment #2: awe-userptr.txt --]
[-- Type: text/plain, Size: 1108 bytes --]

===== sound/oss/awe_wave.c 1.12 vs edited =====
--- 1.12/sound/oss/awe_wave.c	Thu Apr  3 16:35:48 2003
+++ edited/sound/oss/awe_wave.c	Thu Jun  5 16:16:53 2003
@@ -2046,7 +2046,8 @@
 			awe_info.nr_voices = awe_max_voices;
 		else
 			awe_info.nr_voices = AWE_MAX_CHANNELS;
-		memcpy((char*)arg, &awe_info, sizeof(awe_info));
+		if (copy_to_user((char*)arg, &awe_info, sizeof(awe_info)))
+			return -EFAULT;
 		return 0;
 		break;
 
@@ -2063,10 +2064,12 @@
 
 	case SNDCTL_SYNTH_MEMAVL:
 		return memsize - awe_free_mem_ptr() * 2;
+		break;
 
 	default:
 		printk(KERN_WARNING "AWE32: unsupported ioctl %d\n", cmd);
 		return -EINVAL;
+		break;
 	}
 }
 
@@ -4314,7 +4317,8 @@
 	if (((cmd >> 8) & 0xff) != 'M')
 		return -EINVAL;
 
-	level = *(int*)arg;
+	if (get_user(level, (int *)arg))
+		return -EFAULT;
 	level = ((level & 0xff) + (level >> 8)) / 2;
 	DEBUG(0,printk("AWEMix: cmd=%x val=%d\n", cmd & 0xff, level));
 
@@ -4370,7 +4374,9 @@
 		level = 0;
 		break;
 	}
-	return *(int*)arg = level;
+	if (put_user(level, (int *)arg))
+		return -EFAULT;
+	return level;
 }
 #endif /* CONFIG_AWE32_MIXER */
 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [CHECKER][PATCH] awe_wave.c user pointer dereference
  2003-06-05 21:34 [CHECKER][PATCH] awe_wave.c user pointer dereference Hollis Blanchard
@ 2003-06-05 22:07 ` Linus Torvalds
  2003-06-06 16:46   ` __user annotations Hollis Blanchard
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2003-06-05 22:07 UTC (permalink / raw)
  To: Hollis Blanchard; +Cc: linux-kernel


On Thu, 5 Jun 2003, Hollis Blanchard wrote:
>
> Two ioctl functions in sound/oss/awe_wave.c were directly dereferencing 
> a user-supplied pointer in a few places. Please apply.

When you do patches like this, can you please add the "__user" annotations 
while you're at it? Also, if your mailer doesn't rape whitespace, I 
seriously prefer patches in-line in the email, so that I don't have to 
edit the email and can reply to it directly?

		Linus



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-05 22:07 ` Linus Torvalds
@ 2003-06-06 16:46   ` Hollis Blanchard
  2003-06-06 17:28     ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Hollis Blanchard @ 2003-06-06 16:46 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel

On Thursday, Jun 5, 2003, at 17:07 US/Central, Linus Torvalds wrote:
>
> On Thu, 5 Jun 2003, Hollis Blanchard wrote:
>>
>> Two ioctl functions in sound/oss/awe_wave.c were directly 
>> dereferencing
>> a user-supplied pointer in a few places. Please apply.
>
> When you do patches like this, can you please add the "__user" 
> annotations
> while you're at it?

I was hoping for a little more explanation on that before I used it... 
for example, will the following code generate a warning? An error?

void func_b(void *b) { }

void func_a(__user void *a)
{
	func_b(a);
}

How about the other way, passing a normal pointer to a function with 
__user in its prototype?

I'm just worried that as soon as I use __user once, entire call chains 
are going to start spewing warnings/errors.

> Also, if your mailer doesn't rape whitespace, I
> seriously prefer patches in-line in the email, so that I don't have to
> edit the email and can reply to it directly?

I was under the impression that text/plain attachments were ok with 
you, but it looks like inline will work too.

-- 
Hollis Blanchard
IBM Linux Technology Center


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-06 16:46   ` __user annotations Hollis Blanchard
@ 2003-06-06 17:28     ` Linus Torvalds
  2003-06-07  0:32       ` Paul Mackerras
  2003-06-07 12:32       ` Ingo Oeser
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2003-06-06 17:28 UTC (permalink / raw)
  To: Hollis Blanchard; +Cc: linux-kernel


On Fri, 6 Jun 2003, Hollis Blanchard wrote:
> 
> I was hoping for a little more explanation on that before I used it... 
> for example, will the following code generate a warning? An error?

A warning. But only if you have "check" installed, and do the kernel build 
with "C=1". You can build the whole kernel that way, just do a

	"make C=1 bzImage >& make-output"

and you'll still get a fully functional kernel as a result, but you'll 
also get a whole lot of warnings for drivers etc that haven't been 
annotated.

The nice thing about the annotations is that not only do they give proper 
warnings for the whole call-chain if there is something strange, they 
actually make the source code more readable. You no longer worry about 
whether a pointer is a user pointer or a kernel pointer - it's obvious 
from the sources.

> void func_b(void *b) { }
> 
> void func_a(__user void *a)
> {
> 	func_b(a);
> }
> 
> How about the other way, passing a normal pointer to a function with 
> __user in its prototype?

Also a warning. For example, doing a

	make C=1 sound/oss/awe_wave.o

results in this output:

	  CHECK   sound/oss/awe_wave.c
	warning: sound/oss/awe_wave.c:2049:21: incorrect type in argument 1 (different address spaces)
	warning: sound/oss/awe_wave.c:2049:21:   expected void [noderef] *to<asn:1>
	warning: sound/oss/awe_wave.c:2049:21:   got char *
	warning: sound/oss/awe_wave.c:2855:50: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:2855:50:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:2855:50:   got char const *addr
	warning: sound/oss/awe_wave.c:3046:33: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3046:33:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3046:33:   got char const *addr
	warning: sound/oss/awe_wave.c:3155:32: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3155:32:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3155:32:   got char const *addr
	warning: sound/oss/awe_wave.c:3291:39: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3291:39:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3291:39:   got char const *addr
	warning: sound/oss/awe_wave.c:3338:36: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3338:36:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3338:36:   got char const *addr
	warning: sound/oss/awe_wave.c:3397:35: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3397:35:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3397:35:   got char const *addr
	warning: sound/oss/awe_wave.c:3454:35: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3454:35:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3454:35:   got char const *addr
	warning: sound/oss/awe_wave.c:3673:50: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:3673:50:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:3673:50:   got char const *addr
	warning: sound/oss/awe_wave.c:4964:55: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:4964:55:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:4964:55:   got char const *addr
	warning: sound/oss/awe_wave.c:5072:55: incorrect type in argument 2 (different address spaces)
	warning: sound/oss/awe_wave.c:5072:55:   expected void const [noderef] *from<asn:1>
	warning: sound/oss/awe_wave.c:5072:55:   got char const *addr
	  CC      sound/oss/awe_wave.o

(Right now, "check" does NOT warn about dereferencing a __user pointer 
directly, that will require it to walk the expression tree a second time 
after evaluating the types, and I've been lazy. But the capability is 
there, that's why __user pointers are marked [noderef].).

> I'm just worried that as soon as I use __user once, entire call chains 
> are going to start spewing warnings/errors.

Absolutely. They largely already do - I cleaned up the code kernel files, 
but I didn't have the energy to clean up drivers.

HOWEVER, usually it's very obvious to fix the whole chain, unless some
type is sometimes used for kernel addresses and sometimes for user 
addresses (which networking does with iovec's, for example).

And "check" does give pretty good error messages, pointing out exactly 
which argument it doesn't like, and why, and where the original 
declaration was if there are declaration conflicts etc. That is, after 
all, the whole point of "check".

You can get check from

	bk://kernel.bkbits.net/torvalds/sparse

if you want to play with it.

> I was under the impression that text/plain attachments were ok with 
> you, but it looks like inline will work too.

text/plain is _workable_ (so I did actually apply the patch), but inline
is preferred.

		Linus


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-06 17:28     ` Linus Torvalds
@ 2003-06-07  0:32       ` Paul Mackerras
  2003-06-07  0:42         ` Sam Ravnborg
  2003-06-07  0:43         ` Linus Torvalds
  2003-06-07 12:32       ` Ingo Oeser
  1 sibling, 2 replies; 17+ messages in thread
From: Paul Mackerras @ 2003-06-07  0:32 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel

Linus Torvalds writes:

> You can get check from
> 
> 	bk://kernel.bkbits.net/torvalds/sparse

Is that up to date?  I cloned that repository and said "make" and got
heaps of compile errors.  First there were a heap of warnings like
this:

symbol.h:73: warning: declaration does not declare anything

and then lots of errors like this:

parse.c:44: error: structure has no member named `ctype'

and indeed the structures defined in the headers don't seem to match
up with their uses in the .c files.

Paul.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  0:32       ` Paul Mackerras
@ 2003-06-07  0:42         ` Sam Ravnborg
  2003-06-07  0:52           ` Paul Mackerras
  2003-06-07  0:43         ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Sam Ravnborg @ 2003-06-07  0:42 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Linus Torvalds, linux-kernel

On Sat, Jun 07, 2003 at 10:32:04AM +1000, Paul Mackerras wrote:
> Linus Torvalds writes:
> 
> > You can get check from
> > 
> > 	bk://kernel.bkbits.net/torvalds/sparse
> 
> Is that up to date?  I cloned that repository and said "make" and got
> heaps of compile errors.

Cloned it today - compiled without a single warning.
(RH8.0, gcc 3.2)

>  First there were a heap of warnings like
> this:
> 
> symbol.h:73: warning: declaration does not declare anything
My version looks like this:
        struct preprocessor_sym {
                struct token *expansion;
                struct token *arglist;
        };  <= line 73

        struct ctype_sym {

Looks like something wrong at your side...

	Sam

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  0:32       ` Paul Mackerras
  2003-06-07  0:42         ` Sam Ravnborg
@ 2003-06-07  0:43         ` Linus Torvalds
  2003-06-07  1:06           ` Arnaldo Carvalho de Melo
  2003-06-07 16:49           ` Daniel Jacobowitz
  1 sibling, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2003-06-07  0:43 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linux-kernel


On Sat, 7 Jun 2003, Paul Mackerras wrote:
> Linus Torvalds writes:
> 
> > You can get check from
> > 
> > 	bk://kernel.bkbits.net/torvalds/sparse
> 
> Is that up to date?  I cloned that repository and said "make" and got
> heaps of compile errors.  First there were a heap of warnings like
> this:

You need to have a modern compiler. The "heaps of errors" is what you get 
if you use a stone-age compiler that doesn't support anonymous structure 
and union members or other C99 features.

Gcc has supported them since some pre-3.x version (which is pretty late,
since they've been around in other compilers for much longer). They are a
great way to make readable data structures that have internal structure
_without_ having to have that structure show up unnecessarily in usage.

		Linus


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  0:42         ` Sam Ravnborg
@ 2003-06-07  0:52           ` Paul Mackerras
  0 siblings, 0 replies; 17+ messages in thread
From: Paul Mackerras @ 2003-06-07  0:52 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Linus Torvalds, linux-kernel

Sam Ravnborg writes:

> Cloned it today - compiled without a single warning.
> (RH8.0, gcc 3.2)

Hmmm, I am using gcc 3.3 (debian sid).

> My version looks like this:
>         struct preprocessor_sym {
>                 struct token *expansion;
>                 struct token *arglist;
>         };  <= line 73

So does mine.

>         struct ctype_sym {
> 
> Looks like something wrong at your side...

Well, does symbol.h have a `ctype' member in your copy?  It doesn't in
mine.  And line 44 of parse.c says:

        sym->ctype.base_type = ctype->base_type;

which fails because sym is a struct symbol * and struct symbol doesn't
have a ctype member.  And FWIW the compilation fails with gcc-3.2 too.

I have included the version of symbol.h that I have below.  Could you
compare that against yours and/or do a pull and see if it still works
for you?

Paul.

#ifndef SYMBOL_H
#define SYMBOL_H
/*
 * Basic symbol and namespace definitions.
 *
 * Copyright (C) 2003 Transmeta Corp.
 *
 *  Licensed under the Open Software License version 1.1
 */

#include "token.h"

/*
 * An identifier with semantic meaning is a "symbol".
 *
 * There's a 1:n relationship: each symbol is always
 * associated with one identifier, while each identifier
 * can have one or more semantic meanings due to C scope
 * rules.
 *
 * The progression is symbol -> token -> identifier. The
 * token contains the information on where the symbol was
 * declared.
 */
enum namespace {
	NS_NONE,
	NS_PREPROCESSOR,
	NS_TYPEDEF,
	NS_STRUCT,
	NS_ENUM,
	NS_LABEL,
	NS_SYMBOL,
	NS_ITERATOR,
};

enum type {
	SYM_BASETYPE,
	SYM_NODE,
	SYM_PTR,
	SYM_FN,
	SYM_ARRAY,
	SYM_STRUCT,
	SYM_UNION,
	SYM_ENUM,
	SYM_TYPEDEF,
	SYM_TYPEOF,
	SYM_MEMBER,
	SYM_BITFIELD,
	SYM_LABEL,
};

struct ctype {
	unsigned long modifiers;
	unsigned long alignment;
	unsigned int contextmask, context, as;
	struct symbol *base_type;
};

struct symbol {
	enum namespace namespace:8;
	enum type type:8;
	struct position pos;		/* Where this symbol was declared */
	struct ident *ident;		/* What identifier this symbol is associated with */
	struct symbol *next_id;		/* Next semantic symbol that shares this identifier */
	struct symbol **id_list;	/* Backpointer to symbol list head */
	struct scope	*scope;
	struct symbol	*same_symbol;
	int (*evaluate)(struct expression *);

	struct preprocessor_sym {
		struct token *expansion;
		struct token *arglist;
	};
	
	struct ctype_sym {
		unsigned long	offset;
		unsigned int	bit_size;
		unsigned int	bit_offset:8,
				fieldwidth:8,
				arg_count:10,
				variadic:1,
				used:1,
				initialized:1;
		int	array_size;
		struct ctype ctype;
		struct symbol_list *arguments;
		struct statement *stmt;
		struct symbol_list *symbol_list;
		struct expression *initializer;
		long long value;		/* Initial value */
	};
	void *aux;				/* Auxiliary info, eg. backend information */
};

/* Modifiers */
#define MOD_AUTO	0x0001
#define MOD_REGISTER	0x0002
#define MOD_STATIC	0x0004
#define MOD_EXTERN	0x0008

#define MOD_STORAGE	(MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL)

#define MOD_CONST	0x0010
#define MOD_VOLATILE	0x0020
#define MOD_SIGNED	0x0040
#define MOD_UNSIGNED	0x0080

#define MOD_CHAR	0x0100
#define MOD_SHORT	0x0200
#define MOD_LONG	0x0400
#define MOD_LONGLONG	0x0800

#define MOD_TYPEDEF	0x1000
#define MOD_STRUCTOF	0x2000
#define MOD_UNIONOF	0x4000
#define MOD_ENUMOF	0x8000

#define MOD_TYPEOF	0x10000
#define MOD_ATTRIBUTE	0x20000
#define MOD_INLINE	0x40000
#define MOD_ADDRESSABLE	0x80000

#define MOD_NOCAST	0x100000
#define MOD_NODEREF	0x200000
#define MOD_ACCESSED	0x400000
#define MOD_TOPLEVEL	0x800000	// scoping..

#define MOD_LABEL	0x1000000

/* Basic types */
extern struct symbol	void_type,
			int_type,
			label_type,
			fp_type,
			vector_type,
			bad_type;

/* C types */
extern struct symbol	bool_ctype, void_ctype,
			char_ctype, uchar_ctype,
			short_ctype, ushort_ctype,
			int_ctype, uint_ctype,
			long_ctype, ulong_ctype,
			llong_ctype, ullong_ctype,
			float_ctype, double_ctype, ldouble_ctype,
			string_ctype, ptr_ctype, label_type;


/* Basic identifiers */
extern struct ident	sizeof_ident,
			alignof_ident,
			__alignof_ident,
			__alignof___ident,
			if_ident,
			else_ident,
			switch_ident,
			case_ident,
			default_ident,
			break_ident,
			continue_ident,
			for_ident,
			while_ident,
			do_ident,
			goto_ident,
			return_ident;

extern struct ident	__asm___ident,
			__asm_ident,
			asm_ident,
			__volatile___ident,
			__volatile_ident,
			volatile_ident,
			__attribute___ident,
			__attribute_ident,
			pragma_ident;

#define symbol_is_typename(sym) ((sym)->type == SYM_TYPE)

extern struct symbol_list *used_list;

extern void access_symbol(struct symbol *);


extern struct symbol *lookup_symbol(struct ident *, enum namespace);
extern void init_symbols(void);
extern struct symbol *alloc_symbol(struct position, int type);
extern void show_type(struct symbol *);
extern const char *modifier_string(unsigned long mod);
extern void show_symbol(struct symbol *);
extern void show_type_list(struct symbol *);
extern void show_symbol_list(struct symbol_list *, const char *);
extern void add_symbol(struct symbol_list **, struct symbol *);
extern void bind_symbol(struct symbol *, struct ident *, enum namespace);

extern struct symbol *examine_symbol_type(struct symbol *);
extern void examine_simple_symbol_type(struct symbol *);
extern const char *show_typename(struct symbol *sym);

extern void debug_symbol(struct symbol *);
extern void merge_type(struct symbol *sym, struct symbol *base_type);
extern void check_declaration(struct symbol *sym);

#endif /* SEMANTIC_H */

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  0:43         ` Linus Torvalds
@ 2003-06-07  1:06           ` Arnaldo Carvalho de Melo
  2003-06-07  1:09             ` Paul Mackerras
  2003-06-07 16:49           ` Daniel Jacobowitz
  1 sibling, 1 reply; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2003-06-07  1:06 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Paul Mackerras, linux-kernel

Em Fri, Jun 06, 2003 at 05:43:58PM -0700, Linus Torvalds escreveu:
> 
> On Sat, 7 Jun 2003, Paul Mackerras wrote:
> > Linus Torvalds writes:
> > 
> > > You can get check from
> > > 
> > > 	bk://kernel.bkbits.net/torvalds/sparse
> > 
> > Is that up to date?  I cloned that repository and said "make" and got
> > heaps of compile errors.  First there were a heap of warnings like
> > this:
> 
> You need to have a modern compiler. The "heaps of errors" is what you get 
> if you use a stone-age compiler that doesn't support anonymous structure 
> and union members or other C99 features.
> 
> Gcc has supported them since some pre-3.x version (which is pretty late,
> since they've been around in other compilers for much longer). They are a
> great way to make readable data structures that have internal structure
> _without_ having to have that structure show up unnecessarily in usage.

In 3.3 this style is not accepted (haven't read the c99 draft to see if it is
OK)

struct foo {
	struct bar {
		int a, b;
	};
}

But this one is:

struct foo {
	struct /* bar */ {
		int a, b;
	}
}

Does anybody knows if gcc 3.3 behaviour is correct and the fact that gcc 3.2
accepts the former style was just a temporary bug?

- Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  1:06           ` Arnaldo Carvalho de Melo
@ 2003-06-07  1:09             ` Paul Mackerras
  0 siblings, 0 replies; 17+ messages in thread
From: Paul Mackerras @ 2003-06-07  1:09 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Linus Torvalds, linux-kernel

Arnaldo Carvalho de Melo writes:

> In 3.3 this style is not accepted (haven't read the c99 draft to see if it is
> OK)

Ah, thank you, that explains it, since gcc-3.3 is now the default in
debian sid.  In fact it does compile OK for me with gcc-3.2.  (I
previously said it didn't but that was because I said `CC=gcc-3.2
make' not 'make CC=gcc-3.2'.)

Paul.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-06 17:28     ` Linus Torvalds
  2003-06-07  0:32       ` Paul Mackerras
@ 2003-06-07 12:32       ` Ingo Oeser
  2003-06-07 16:25         ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Ingo Oeser @ 2003-06-07 12:32 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel

Hi Linus,

On Fri, Jun 06, 2003 at 10:28:22AM -0700, Linus Torvalds wrote:
> HOWEVER, usually it's very obvious to fix the whole chain, unless some
> type is sometimes used for kernel addresses and sometimes for user 
> addresses (which networking does with iovec's, for example).
 
Then it's not very useful for me. I usally define the ABI between
user space and kernel space trough IOCTL like that:

/* These structures are usally bigger and nested deeper */
struct in_foo_ioctl_name {
   int bla;
}

struct out_foo_ioctl_name {
   char blubb;
}

union foo_ioctl_name {
   struct in_foo_ioctl_name in;
   struct out_foo_ioctl_name out;
}

#define SUBSYS_IOCTL 0xee

#define SUBSYS_FOO _IOWR(SUBSYS_IOCTL, 0x1, union foo_ioctl_name)

Now I do in principle

   union foo_ioctl_name k, *u = (union foo_ioctl_name *)arg;

   if (copy_from_user(&k.in, &u, sizeof(k.in)) return -EFAULT;
   if (handle_foo(&k)) return -EINVAL;
   if (copy_to_user(&u, &k.out, sizeof(k.out)) return -EFAULT;
   
which I consider very clean (our project provides both: The
only ABI provider and the only ABI user) and works from 2.0
trough 2.5 so far.

This will NOT work anymore with __user annotations, right?

That's a big pity. How do I workaround this? I would like to
help resolving this issues, if you are interested.

Regards

Ingo Oeser

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07 12:32       ` Ingo Oeser
@ 2003-06-07 16:25         ` Linus Torvalds
  2003-06-07 16:43           ` Sam Ravnborg
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2003-06-07 16:25 UTC (permalink / raw)
  To: Ingo Oeser; +Cc: linux-kernel


On Sat, 7 Jun 2003, Ingo Oeser wrote:
> 
> That's a big pity. How do I workaround this? I would like to
> help resolving this issues, if you are interested.

The solution to these things is to _always_ have a separate type for the 
user thing than for the kernel thing. 

In practice, a lot of code has ended up doing that _anyway_, since the
kernel usually wants to have a few extra fields for its internal use. The
classic unix example of this, of course, is "struct stat" vs "struct
inode".

But if your structures are 100% the same, then you can just share them. 
There's nothing wrong with having

	struct ioctl_arg {
		int value;
		int another_value;
		..
	};

and then in your ioctl routines you have

    int my_ioctl_routine(struct ioctl_arg __user *ptr)
    {
	struct ioctl_arg arg;

	if (copy_from_user(&arg, ptr, sizeof(*ptr))
		return -EFAULT;
	...
    }

and that's fine.

You can even have user pointers _inside_ the structure: because "sparse" 
really understands C types at a very fundamental level (like a compiler 
would, not like some simpler source scanner), you can have

	struct ioctl_arg {
		int value;
		void __user *buf;
	};

and do

    int my_ioctl_routine(struct ioctl_arg __user *ptr)
    {
	struct ioctl_arg arg;
	char buffer[10];

	if (copy_from_user(&arg, ptr, sizeof(*ptr))
		return -EFAULT;

and sparse will be aware of the fact that "arg.buf" is a user pointer, and 
it will properly warn if you pass it to a function that expects a kernel 
pointer (or assign it to a normal non-user pointer thing).

		Linus


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07 16:25         ` Linus Torvalds
@ 2003-06-07 16:43           ` Sam Ravnborg
  2003-06-07 16:48             ` Sam Ravnborg
  0 siblings, 1 reply; 17+ messages in thread
From: Sam Ravnborg @ 2003-06-07 16:43 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Ingo Oeser, linux-kernel

On Sat, Jun 07, 2003 at 09:25:43AM -0700, Linus Torvalds wrote:
> 
> You can even have user pointers _inside_ the structure: because "sparse" 

Since we start to know your checker by the name sparse, why not
call the default executable sparse?

	Sam

===== Makefile 1.21 vs edited =====
--- 1.21/Makefile	Tue Jun  3 03:15:27 2003
+++ edited/Makefile	Sat Jun  7 18:41:15 2003
@@ -2,7 +2,7 @@
 CFLAGS=-g -Wall
 AR=ar
 
-PROGRAMS=test-lexing test-parsing obfuscate check
+PROGRAMS=test-lexing test-parsing obfuscate sparse
 
 LIB_H=    token.h parse.h lib.h symbol.h scope.h expression.h target.h
 
@@ -22,7 +22,7 @@
 obfuscate: obfuscate.o $(LIB_FILE)
 	gcc -o $@ $< $(LIB_FILE)
 
-check: check.o $(LIB_FILE)
+sparse: check.o $(LIB_FILE)
 	gcc -o $@ $< $(LIB_FILE)
 
 $(LIB_FILE): $(LIB_OBJS)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07 16:43           ` Sam Ravnborg
@ 2003-06-07 16:48             ` Sam Ravnborg
  0 siblings, 0 replies; 17+ messages in thread
From: Sam Ravnborg @ 2003-06-07 16:48 UTC (permalink / raw)
  To: Linus Torvalds, Ingo Oeser, linux-kernel

On Sat, Jun 07, 2003 at 06:43:30PM +0200, Sam Ravnborg wrote:
> Since we start to know your checker by the name sparse, why not
> call the default executable sparse?

And then we can update the top-level Makefile in the kernel.

	Sam

Rename Linus' checker tool to sparse, and avoid the hardcoded path.

===== Makefile 1.410 vs edited =====
--- 1.410/Makefile	Tue Jun  3 23:27:14 2003
+++ edited/Makefile	Sat Jun  7 18:44:22 2003
@@ -204,7 +204,7 @@
 DEPMOD		= /sbin/depmod
 KALLSYMS	= scripts/kallsyms
 PERL		= perl
-CHECK		= /home/torvalds/parser/check
+CHECK		= sparse
 MODFLAGS	= -DMODULE
 CFLAGS_MODULE   = $(MODFLAGS)
 AFLAGS_MODULE   = $(MODFLAGS)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07  0:43         ` Linus Torvalds
  2003-06-07  1:06           ` Arnaldo Carvalho de Melo
@ 2003-06-07 16:49           ` Daniel Jacobowitz
  2003-06-08  2:17             ` Paul Mackerras
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Jacobowitz @ 2003-06-07 16:49 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Paul Mackerras, linux-kernel

On Fri, Jun 06, 2003 at 05:43:58PM -0700, Linus Torvalds wrote:
> 
> On Sat, 7 Jun 2003, Paul Mackerras wrote:
> > Linus Torvalds writes:
> > 
> > > You can get check from
> > > 
> > > 	bk://kernel.bkbits.net/torvalds/sparse
> > 
> > Is that up to date?  I cloned that repository and said "make" and got
> > heaps of compile errors.  First there were a heap of warnings like
> > this:
> 
> You need to have a modern compiler. The "heaps of errors" is what you get 
> if you use a stone-age compiler that doesn't support anonymous structure 
> and union members or other C99 features.
> 
> Gcc has supported them since some pre-3.x version (which is pretty late,
> since they've been around in other compilers for much longer). They are a
> great way to make readable data structures that have internal structure
> _without_ having to have that structure show up unnecessarily in usage.

Actually, I believe they are an extension, which GCC honors.  Unnamed
structures in standard C99 are actually declaring an unnamed type, not
an unnamed member.  Try it:

     struct {
       int a;
       union {
         int b;
         float c;
       };
       int d;
     } foo;

int bar()
{
  return foo.a + foo.d + foo.b;
}


With -std=c99, the reference to foo.b is an error; with -std=gnu99 or
-std=gnu89, it is accepted.


I don't know why they were getting rejected for Paul, though.  Did you
have GNU set to -ansi mode?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: __user annotations
  2003-06-07 16:49           ` Daniel Jacobowitz
@ 2003-06-08  2:17             ` Paul Mackerras
  0 siblings, 0 replies; 17+ messages in thread
From: Paul Mackerras @ 2003-06-08  2:17 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: Linus Torvalds, linux-kernel

Daniel Jacobowitz writes:

> I don't know why they were getting rejected for Paul, though.  Did you
> have GNU set to -ansi mode?

I was using gcc-3.3.  I tried both -std=c99 and -std=gnu99, and gcc-3.3
didn't like the code either way, nor did it like it with no -std
option.  Gcc-3.2 compiles it perfectly happily though, so that is what
I am doing for now.

(I did have to hack some of the pre-definitions in check.c to make it
useful on PPC, e.g., I changed it to pre-define __ppc__ instead of
__i386__.)

Paul.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [CHECKER][PATCH] awe_wave.c user pointer dereference
       [not found] <20030605155225.4b1c64b3.akpm@digeo.com>
@ 2003-06-06 16:31 ` Hollis Blanchard
  0 siblings, 0 replies; 17+ messages in thread
From: Hollis Blanchard @ 2003-06-06 16:31 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

On Thursday, Jun 5, 2003, at 17:52 US/Central, Andrew Morton wrote:

> Hollis Blanchard <hollisb@us.ibm.com> wrote:
>>
>> +			return -EFAULT;
>>  		return 0;
>>  		break;
>>
>> @@ -2063,10 +2064,12 @@
>>
>>  	case SNDCTL_SYNTH_MEMAVL:
>>  		return memsize - awe_free_mem_ptr() * 2;
>> +		break;
>>
>>  	default:
>>  		printk(KERN_WARNING "AWE32: unsupported ioctl %d\n", cmd);
>>  		return -EINVAL;
>> +		break;
>
> There's no need for a "break" after a "return"!

I've gotten a couple questions about that. :)

I did it for two reasons: 1) I'm a big fan of consistancy, and 3 of the 
5 ioctl cases already had the break. 2) should the returns be replaced 
with something like "retval = -EINVAL" and a single "return retval" at 
the end of the function, I can easily imagine visually missing the need 
for a break.

However, although a break after a return is fine with gcc -Wall, Joe 
Perches informs me that it will annoy lint and the Intel compiler. So 
here is the updated patch, which removes the (pre-existing! :) breaks 
from the other cases.

-- 
Hollis Blanchard
IBM Linux Technology Center

===== sound/oss/awe_wave.c 1.12 vs edited =====
--- 1.12/sound/oss/awe_wave.c	Thu Apr  3 16:35:48 2003
+++ edited/sound/oss/awe_wave.c	Fri Jun  6 11:17:19 2003
@@ -2046,20 +2046,18 @@
  			awe_info.nr_voices = awe_max_voices;
  		else
  			awe_info.nr_voices = AWE_MAX_CHANNELS;
-		memcpy((char*)arg, &awe_info, sizeof(awe_info));
+		if (copy_to_user((char*)arg, &awe_info, sizeof(awe_info)))
+			return -EFAULT;
  		return 0;
-		break;

  	case SNDCTL_SEQ_RESETSAMPLES:
  		awe_reset(dev);
  		awe_reset_samples();
  		return 0;
-		break;

  	case SNDCTL_SEQ_PERCMODE:
  		/* what's this? */
  		return 0;
-		break;

  	case SNDCTL_SYNTH_MEMAVL:
  		return memsize - awe_free_mem_ptr() * 2;
@@ -4314,7 +4312,8 @@
  	if (((cmd >> 8) & 0xff) != 'M')
  		return -EINVAL;

-	level = *(int*)arg;
+	if (get_user(level, (int *)arg))
+		return -EFAULT;
  	level = ((level & 0xff) + (level >> 8)) / 2;
  	DEBUG(0,printk("AWEMix: cmd=%x val=%d\n", cmd & 0xff, level));

@@ -4370,7 +4369,9 @@
  		level = 0;
  		break;
  	}
-	return *(int*)arg = level;
+	if (put_user(level, (int *)arg))
+		return -EFAULT;
+	return level;
  }
  #endif /* CONFIG_AWE32_MIXER */
  


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2003-06-08  2:07 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-06-05 21:34 [CHECKER][PATCH] awe_wave.c user pointer dereference Hollis Blanchard
2003-06-05 22:07 ` Linus Torvalds
2003-06-06 16:46   ` __user annotations Hollis Blanchard
2003-06-06 17:28     ` Linus Torvalds
2003-06-07  0:32       ` Paul Mackerras
2003-06-07  0:42         ` Sam Ravnborg
2003-06-07  0:52           ` Paul Mackerras
2003-06-07  0:43         ` Linus Torvalds
2003-06-07  1:06           ` Arnaldo Carvalho de Melo
2003-06-07  1:09             ` Paul Mackerras
2003-06-07 16:49           ` Daniel Jacobowitz
2003-06-08  2:17             ` Paul Mackerras
2003-06-07 12:32       ` Ingo Oeser
2003-06-07 16:25         ` Linus Torvalds
2003-06-07 16:43           ` Sam Ravnborg
2003-06-07 16:48             ` Sam Ravnborg
     [not found] <20030605155225.4b1c64b3.akpm@digeo.com>
2003-06-06 16:31 ` [CHECKER][PATCH] awe_wave.c user pointer dereference Hollis Blanchard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).