netdev - Re: [PATCH][CAN]: Fix copy_from_user() results interpretation.

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]

Message-Id: <20080426.000302.177643527.davem@davemloft.net>
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