lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 26 Apr 2008 00:03:02 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	sam@...nborg.org
Cc:	wg@...ndegger.com, oliver.hartkopp@...kswagen.de,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	urs.thuermann@...kswagen.de, xemul@...nvz.org
Subject: Re: [PATCH][CAN]: Fix copy_from_user() results interpretation.

From: Sam Ravnborg <sam@...nborg.org>
Date: Sat, 26 Apr 2008 08:40:07 +0200

> On Sat, Apr 26, 2008 at 08:19:31AM +0200, Wolfgang Grandegger wrote:
> > What about removing the assignment "err =" in that case.
> Preferred.
> 
> See sample patch below (made on top of -linus
> so it does likely not apply but made it only to
> see the difference anyway).

I want to make one comment, directed at Wolfgang.

You are absolutely wrong, Wolfgang, in saying that Pavel's
original patch isn't easier to review than just changing
this code to go:

	if (copy_*_user())
		return -EFAULT;

In fact, recoding things like this is an immense extra hardship on a
reviewer.  I'll explain why.

If I see a patch that changes:

	err = SOMETHING;
	break;

into:

	err = SOMETHING_ELSE;
	break;

I know, WITH JUST READING THE PATCH, exactly what the side effects of
this change are.

I DO NOT need to bring the code into my editor and validate side
effects to the surrounding code.

I know that the assignment to 'err' is being changed, and that's it.

Whereas if you change:

	err = SOMETHING;
	break;

into:

	if (SOMETHING)
		return -SOME_ERROR;
	break;

I now have to bring the code into an editor and make sure that the
control flow change doesn't break things.

For example, maybe the exit of the switch statement was important, to
make sure cleanup code runs at the end of the function to release
locks, free allocated memory, etc.

With Pavel's patch it is not necessary to make such validations so
it's INFINITELY easier to validate.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists