Re: [RFC 0/6] xHCI and USB security bug fixes
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/6] xHCI and USB security bug fixes
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/6] xHCI and USB security bug fixes
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 :) 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. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/6] xHCI and USB security bug fixes
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. 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. Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/6] xHCI and USB security bug fixes
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? Those seems over-eager, and not really needed. Or am I missing somewhere that could change the pointer without the driver knowing it? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 0/6] xHCI and USB security bug fixes
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. Sarah Sharp The following changes since commit c3897aa5386faba77e5bbdf94902a1658d3a5b11: xhci: Disable D3cold for buggy TI redrivers. (2013-05-24 15:23:59 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git ful-klockwork for you to fetch changes up to c3a0a3ea1c2ece9596b6e3ae3fcb4540fc9b6e31: usb: check usb_hub_to_struct_hub() return value (2013-05-24 17:26:48 -0700) Mathias Nyman (4): xhci: Remove BUG in xhci_setup_addressable_virt_dev xhci: remove BUG() in xhci_get_endpoint_type() xhci: check for failed dma pool allocation usb: check usb_hub_to_struct_hub() return value Sarah Sharp (2): xhci: Remove BUG_ON() in xhci_alloc_container_ctx. xhci: Remove BUG_ON in xhci_get_input_control_ctx. drivers/usb/core/hub.c | 107 ++ drivers/usb/host/xhci-dbg.c |5 ++ drivers/usb/host/xhci-mem.c | 65 +- drivers/usb/host/xhci-ring.c |4 + drivers/usb/host/xhci.c | 148 -- 5 files changed, 234 insertions(+), 95 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
