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. > > > 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? 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? Sarah Sharp -- 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
