On Mon, Jun 03, 2013 at 12:10:19PM -0700, Sarah Sharp wrote: > On Thu, May 30, 2013 at 07:04:03AM +0900, Greg Kroah-Hartman wrote: > > On Wed, May 29, 2013 at 10:45:04AM -0700, Sarah Sharp wrote: > > > On Wed, May 29, 2013 at 10:27:50AM +0900, Greg Kroah-Hartman wrote: > > > > On Fri, May 24, 2013 at 05:42:52PM -0700, Sarah Sharp wrote: > > > > > This patchset address some (but not all) of the security issues found > > > > > with the Klockwork static analysis tool. I have not reviewed these in > > > > > detail to see if these could be used by attackers, so someone with more > > > > > security experience may want to look these over. > > > > > > > > A lot of these changes are just to add checks to functions that you are > > > > calling yourself. How can those pointers be "not valid" when you > > > > control what you pass to them? > > > > > > It's purely paranoia. It's entirely possible we'll add new code later > > > that would accidentally trigger these checks. That's especially true > > > of, say, the device speeds, since "USB 3.1" (10Gbps) is in the works. > > > > Then let's worry about it at that point in time :) > > All right. Most of these patches are defensive coding, but you've had > more experience with kernel coding, so I'll leave it up to you. "defensive coding" in the kernel is only to be done when you are taking data from an "untrusted" place, like userspace, or if the pointer or value could have changed due to others doing things with it that this function doesn't know about. For internal function calls, where we control all of the callers, it doesn't make sense to validate all paramaters passed to us, otherwise the size of the kernel would grow to be huge with unneeded checks on almost every function call. That's how we stay lean, or at least attempt to stay lean :) > > > > Those seems over-eager, and not really needed. Or am I missing > > > > somewhere that could change the pointer without the driver knowing it? > > > > > > In all honesty, these patches are the result of a bureaucratic push for > > > "code quality". We switched static analysis tools from Coverity to > > > Klockwork, and the QA folks pushed us to fix the "issues" that Klockwork > > > discovered. > > > > > > If you don't think they're appropriate, let me know, and I'll push back. > > > > It is great to get rid of the BUG_ON() calls. But to be over-eager in > > checking parameters that we have full control of is not needed at all. > > > > So if you rework the patches to just clean up the BUG_ON() calls, I'll > > be glad to accept them, but they aren't "security" issues at all. > > Ok, so your opinion is that most of these patches shouldn't go into > stable? As they don't fix a bug, or a problem that anyone can ever hit, why would they go into stable? > I can certainly rework the patch descriptions to remove the > stable references, and send them as a usb-next pull request instead. > > If you're ok with taking BUG() removal patches, it seems like: > > - Patch 1 is fine, since it removes a BUG() call. > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=d63f9bd7a0a3b5a6993cacf5a26e5f10a784ebbb > > - Patch 2 is defensive coding against adding a new xHCI context type, > mixed with some changes to functions to make them accept > xhci_input_control_ctx structures instead of xhci_container_ctx > structures, so that xhci_get_input_control_ctx() doesn't need to be > called as often. Do you want to take the former part? If not, I > feel at least the latter part would be a good candidate for usb-next. > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=9edeae7ac97d63d68ebb2ae008edc4ede93855db > > - Patch 3 seems fine to me for usb-next, since it removes a BUG() and > condenses two switch statements into one. > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=168faa7f1cb2170784671a2acc6cfee981b9936c > > - Patch 4 is defensive coding against adding a new endpoint type, but it > also removes a BUG() call. How would you like to see this patch > reworked? > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=35c5e886fc18dc43447be0a2f47f0bbd6b2ef9a4 > > - Patch 5 seems fine to me, since it catches the case where the kernel > is out of memory and DMA pool allocation fails. > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=6aaa3a3315eb15fb36e49055db4019587063a1ee > > - Patch 6 needs to be reworked with Alan's suggestions. > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?id=c3a0a3ea1c2ece9596b6e3ae3fcb4540fc9b6e31 > > Do you agree with this assessment? What do you suggest we do with > patches 2 and 4? I don't remember them exactly, how about you rework them how you feel they should be done, and resend and I'll be glad to review them again? thanks, greg "drowning in patches" k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
