Finding user/kernel pointer bugs [no html]

linux-usb-devel Luca Risolia 3 participants 4 messages Jun 11, 2004 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Luca Risolia

author

2004-06-11 04:25 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Review

Quoted 4 lines

unsigned int cmd, void* arg)
{
(quoted diff) +1 -0
 	struct w9968cf_device* cam;
+	void __user *user_arg = (void __user *)arg;

The right place to apply this patch is in video_usercopy().

When video_usercopy() is used in the ioctl() method, there is no need to dereference the arg pointer in ioctl() itself, since one of the purposes of video_usercopy() is to do this work for us.

Please have a look at definition of the function in videodev.c.

int
video_usercopy(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg,
int (*func)(struct inode *inode, struct file *file,
unsigned int cmd, unsigned void *arg))

What you have patched in your patch is the function pointed by "func" above, which should already receive the __user pointer to arg.

Luca
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQFAyTWLmdpdKvzmNaQRAhDuAJ0e8okB48LQUaHnxZsSS0ZFTmuxRQCgnYqZ WECeSegLUG5WDyUArFcByKU=
=u+vB
-----END PGP SIGNATURE-----

[email protected]

2004-06-11 04:52 UTC #2 lore raw

Quoted 9 lines · Luca Risolia

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Review
Quoted 4 lines
unsigned int cmd, void* arg)
{
(quoted diff) +1 -0
 	struct w9968cf_device* cam;
+	void __user *user_arg = (void __user *)arg;
The right place to apply this patch is in video_usercopy().

The right thing to do is to kill video_usercopy() as ugly piece of crap.

Greg KH

2004-06-11 16:33 UTC #3 lore raw

In reply to Luca Risolia

Quoted 6 lines · Luca Risolia

Review
Quoted 4 lines
unsigned int cmd, void* arg)
{
(quoted diff) +1 -0
 	struct w9968cf_device* cam;
+	void __user *user_arg = (void __user *)arg;
The right place to apply this patch is in video_usercopy().

Um, the driver you just refered to does not use the video_usercopy() function so your email doesn't make much sense in this context.

Quoted 1 line

Please have a look at definition of the function in videodev.c.

Please excuse me while I go get sick...

Anyway, that function needs to be properly marked up with __user if you want it to live.

good luck,

greg k-h

Luca Risolia

author

2004-06-11 16:42 UTC #4 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 10 lines · On Fri, 11 Jun 2004 09:17:48 -0700 Greg KH <[email protected]> wrote:

Quoted 6 lines · Luca Risolia
Review
Quoted 4 lines
unsigned int cmd, void* arg)
{
(quoted diff) +1 -0
 	struct w9968cf_device* cam;
+	void __user *user_arg = (void __user *)arg;
The right place to apply this patch is in video_usercopy().
Um, the driver you just refered to does not use the video_usercopy() function so your email doesn't make much sense in this context.

Oops, sorry. I forgot the w9968cf doesn't actually use video_usercopy(). However, apart from the "__user" context, there are several drivers under drivers/usb/media/ that still use that usercopy() thing.

Quoted 12 lines

Quoted 2 lines
Please have a look at definition of the function in videodev.c.
Please excuse me while I go get sick...
Anyway, that function needs to be properly marked up with __user if you want it to live.
good luck,
greg k-h

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQFAyeHImdpdKvzmNaQRAgSbAJ9J+Zq4PsS59Z0muH1nJM036CCBzACglYQO 5/kAGoFHru+NpJ0/wNd0YT0=
=57yE
-----END PGP SIGNATURE-----