| Message ID | 20200325161249.55095-21-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
On Wed, Mar 25, 2020 at 5:14 PM <glider@google.com> wrote: > > Depending on the value of is_out kmsan_handle_urb() KMSAN either > marks the data copied to the kernel from a USB device as initialized, > or checks the data sent to the device for being initialized. > > Signed-off-by: Alexander Potapenko <glider@google.com> > To: Alexander Potapenko <glider@google.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Vegard Nossum <vegard.nossum@oracle.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Marco Elver <elver@google.com> > Cc: Andrey Konovalov <andreyknvl@google.com> > Cc: linux-mm@kvack.org > > --- > > This patch was previously called "kmsan: call KMSAN hooks where needed" > > v4: > - split this patch away > > Change-Id: Idd0f8ce858975112285706ffb7286f570bd3007b > --- > drivers/usb/core/urb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > index da923ec176122..4a0b0ac0f52f9 100644 > --- a/drivers/usb/core/urb.c > +++ b/drivers/usb/core/urb.c > @@ -8,6 +8,7 @@ > #include <linux/bitops.h> > #include <linux/slab.h> > #include <linux/log2.h> > +#include <linux/kmsan-checks.h> > #include <linux/usb.h> > #include <linux/wait.h> > #include <linux/usb/hcd.h> > @@ -402,6 +403,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL | > URB_DMA_SG_COMBINED); > urb->transfer_flags |= (is_out ? URB_DIR_OUT : URB_DIR_IN); > + kmsan_handle_urb(urb, is_out); I guess this could simply accept urb and then check urb->transfer_flags instead of also accepting is_out? Alan, do you think this is a good place for a call to kmsan_handle_urb(), which is supposed to check that the memory we pass to a USB device is initialized (so we don't leak uninitialized memory) and mark memory received from the device as initialized? You can find the implementation here: https://github.com/google/kmsan/commit/491a67cf03fa9e0f240fd6eb53a6074e4bfd1a2c#diff-020c941e2b8fc67f5ddca598cd954d57R322 > > if (xfertype != USB_ENDPOINT_XFER_CONTROL && > dev->state < USB_STATE_CONFIGURED) > -- > 2.25.1.696.g5e7596f4ac-goog >
On Tue, 14 Apr 2020, Andrey Konovalov wrote: > On Wed, Mar 25, 2020 at 5:14 PM <glider@google.com> wrote: > > > > Depending on the value of is_out kmsan_handle_urb() KMSAN either > > marks the data copied to the kernel from a USB device as initialized, > > or checks the data sent to the device for being initialized. > > > > Signed-off-by: Alexander Potapenko <glider@google.com> > > To: Alexander Potapenko <glider@google.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Cc: Petr Mladek <pmladek@suse.com> > > Cc: Vegard Nossum <vegard.nossum@oracle.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: Marco Elver <elver@google.com> > > Cc: Andrey Konovalov <andreyknvl@google.com> > > Cc: linux-mm@kvack.org > > > > --- > > > > This patch was previously called "kmsan: call KMSAN hooks where needed" > > > > v4: > > - split this patch away > > > > Change-Id: Idd0f8ce858975112285706ffb7286f570bd3007b > > --- > > drivers/usb/core/urb.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > > index da923ec176122..4a0b0ac0f52f9 100644 > > --- a/drivers/usb/core/urb.c > > +++ b/drivers/usb/core/urb.c > > @@ -8,6 +8,7 @@ > > #include <linux/bitops.h> > > #include <linux/slab.h> > > #include <linux/log2.h> > > +#include <linux/kmsan-checks.h> > > #include <linux/usb.h> > > #include <linux/wait.h> > > #include <linux/usb/hcd.h> > > @@ -402,6 +403,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > > URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL | > > URB_DMA_SG_COMBINED); > > urb->transfer_flags |= (is_out ? URB_DIR_OUT : URB_DIR_IN); > > + kmsan_handle_urb(urb, is_out); > > I guess this could simply accept urb and then check > urb->transfer_flags instead of also accepting is_out? > > Alan, do you think this is a good place for a call to > kmsan_handle_urb(), which is supposed to check that the memory we pass > to a USB device is initialized (so we don't leak uninitialized memory) > and mark memory received from the device as initialized? You can find > the implementation here: > > https://github.com/google/kmsan/commit/491a67cf03fa9e0f240fd6eb53a6074e4bfd1a2c#diff-020c941e2b8fc67f5ddca598cd954d57R322 This has got a couple of problems. Firstly, for control URBs it doesn't check urb->setup_packet, which should always be initialized regardless of the direction because it always gets sent to the device. Secondly, some URBs use scatter-gather transfers, and they don't always store the buffer address in urb->transfer_buffer (indeed, sometimes the buffer is located outside of the kernel's address map). Instead they use urb->sg and urb->num_sgs. To get an idea for how it all works, look at usb_hcd_map_urb_for_dma() in hcd.c. Thirdly, the information we get back from the device doesn't always cover the entire transfer buffer; sometimes the device sends less data than we asked for. Perhaps you don't care very much about this case. Alan Stern
On Tue, Apr 14, 2020 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, 14 Apr 2020, Andrey Konovalov wrote: > > > On Wed, Mar 25, 2020 at 5:14 PM <glider@google.com> wrote: > > > > > > Depending on the value of is_out kmsan_handle_urb() KMSAN either > > > marks the data copied to the kernel from a USB device as initialized, > > > or checks the data sent to the device for being initialized. > > > > > > Signed-off-by: Alexander Potapenko <glider@google.com> > > > To: Alexander Potapenko <glider@google.com> > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: Eric Dumazet <edumazet@google.com> > > > Cc: Wolfram Sang <wsa@the-dreams.de> > > > Cc: Petr Mladek <pmladek@suse.com> > > > Cc: Vegard Nossum <vegard.nossum@oracle.com> > > > Cc: Dmitry Vyukov <dvyukov@google.com> > > > Cc: Marco Elver <elver@google.com> > > > Cc: Andrey Konovalov <andreyknvl@google.com> > > > Cc: linux-mm@kvack.org > > > > > > --- > > > > > > This patch was previously called "kmsan: call KMSAN hooks where needed" > > > > > > v4: > > > - split this patch away > > > > > > Change-Id: Idd0f8ce858975112285706ffb7286f570bd3007b > > > --- > > > drivers/usb/core/urb.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > > > index da923ec176122..4a0b0ac0f52f9 100644 > > > --- a/drivers/usb/core/urb.c > > > +++ b/drivers/usb/core/urb.c > > > @@ -8,6 +8,7 @@ > > > #include <linux/bitops.h> > > > #include <linux/slab.h> > > > #include <linux/log2.h> > > > +#include <linux/kmsan-checks.h> > > > #include <linux/usb.h> > > > #include <linux/wait.h> > > > #include <linux/usb/hcd.h> > > > @@ -402,6 +403,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > > > URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL | > > > URB_DMA_SG_COMBINED); > > > urb->transfer_flags |= (is_out ? URB_DIR_OUT : URB_DIR_IN); > > > + kmsan_handle_urb(urb, is_out); > > > > I guess this could simply accept urb and then check > > urb->transfer_flags instead of also accepting is_out? > > > > Alan, do you think this is a good place for a call to > > kmsan_handle_urb(), which is supposed to check that the memory we pass > > to a USB device is initialized (so we don't leak uninitialized memory) > > and mark memory received from the device as initialized? You can find > > the implementation here: > > > > https://github.com/google/kmsan/commit/491a67cf03fa9e0f240fd6eb53a6074e4bfd1a2c#diff-020c941e2b8fc67f5ddca598cd954d57R322 > > This has got a couple of problems. > > Firstly, for control URBs it doesn't check urb->setup_packet, which > should always be initialized regardless of the direction because it > always gets sent to the device. Ah, yes, we actually had an info-leak report for a bug in the sound subsystem that was leaking data this way, which was misattributed to raw-gadget. This makes sense to add and seems quite easy. > Secondly, some URBs use scatter-gather transfers, and they don't always > store the buffer address in urb->transfer_buffer (indeed, sometimes the > buffer is located outside of the kernel's address map). Instead they > use urb->sg and urb->num_sgs. To get an idea for how it all works, > look at usb_hcd_map_urb_for_dma() in hcd.c. Oh, this is something we need to look into. > Thirdly, the information we get back from the device doesn't always > cover the entire transfer buffer; sometimes the device sends less data > than we asked for. Perhaps you don't care very much about this case. So you mean that we should look at actual_length rather than transfer_buffer_length when initializing memory that comes from the device? Thank you, Alan!
On Tue, 14 Apr 2020, Andrey Konovalov wrote: > On Tue, Apr 14, 2020 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > This has got a couple of problems. > > > > Firstly, for control URBs it doesn't check urb->setup_packet, which > > should always be initialized regardless of the direction because it > > always gets sent to the device. > > Ah, yes, we actually had an info-leak report for a bug in the sound > subsystem that was leaking data this way, which was misattributed to > raw-gadget. This makes sense to add and seems quite easy. > > > Secondly, some URBs use scatter-gather transfers, and they don't always > > store the buffer address in urb->transfer_buffer (indeed, sometimes the > > buffer is located outside of the kernel's address map). Instead they > > use urb->sg and urb->num_sgs. To get an idea for how it all works, > > look at usb_hcd_map_urb_for_dma() in hcd.c. > > Oh, this is something we need to look into. > > > Thirdly, the information we get back from the device doesn't always > > cover the entire transfer buffer; sometimes the device sends less data > > than we asked for. Perhaps you don't care very much about this case. > > So you mean that we should look at actual_length rather than > transfer_buffer_length when initializing memory that comes from the > device? Yes, in theory. Isochronous transfers are difficult, because the data doesn't have to be contiguous in memory. If you want to handle them properly, you have to loop through the entries in the urb->iso_frame_desc[] array and treat them individually. Alan Stern
On Tue, Apr 14, 2020 at 10:45 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, 14 Apr 2020, Andrey Konovalov wrote: > > > On Tue, Apr 14, 2020 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > This has got a couple of problems. > > > > > > Firstly, for control URBs it doesn't check urb->setup_packet, which > > > should always be initialized regardless of the direction because it > > > always gets sent to the device. > > > > Ah, yes, we actually had an info-leak report for a bug in the sound > > subsystem that was leaking data this way, which was misattributed to > > raw-gadget. This makes sense to add and seems quite easy. > > > > > Secondly, some URBs use scatter-gather transfers, and they don't always > > > store the buffer address in urb->transfer_buffer (indeed, sometimes the > > > buffer is located outside of the kernel's address map). Instead they > > > use urb->sg and urb->num_sgs. To get an idea for how it all works, > > > look at usb_hcd_map_urb_for_dma() in hcd.c. > > > > Oh, this is something we need to look into. > > > > > Thirdly, the information we get back from the device doesn't always > > > cover the entire transfer buffer; sometimes the device sends less data > > > than we asked for. Perhaps you don't care very much about this case. > > > > So you mean that we should look at actual_length rather than > > transfer_buffer_length when initializing memory that comes from the > > device? > > Yes, in theory. Isochronous transfers are difficult, because the data > doesn't have to be contiguous in memory. If you want to handle them > properly, you have to loop through the entries in the > urb->iso_frame_desc[] array and treat them individually. > Thanks! I'll take a closer look and will probably ask you more questions, as we're extensively fuzzing USB on syzbot. However for the first stage of upstreaming KMSAN I think we'd better drop USB checks to reduce the patch set. It has already become enormous and hard to review or manage.
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index da923ec176122..4a0b0ac0f52f9 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -8,6 +8,7 @@ #include <linux/bitops.h> #include <linux/slab.h> #include <linux/log2.h> +#include <linux/kmsan-checks.h> #include <linux/usb.h> #include <linux/wait.h> #include <linux/usb/hcd.h> @@ -402,6 +403,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL | URB_DMA_SG_COMBINED); urb->transfer_flags |= (is_out ? URB_DIR_OUT : URB_DIR_IN); + kmsan_handle_urb(urb, is_out); if (xfertype != USB_ENDPOINT_XFER_CONTROL && dev->state < USB_STATE_CONFIGURED)
Depending on the value of is_out kmsan_handle_urb() KMSAN either marks the data copied to the kernel from a USB device as initialized, or checks the data sent to the device for being initialized. Signed-off-by: Alexander Potapenko <glider@google.com> To: Alexander Potapenko <glider@google.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: Petr Mladek <pmladek@suse.com> Cc: Vegard Nossum <vegard.nossum@oracle.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Marco Elver <elver@google.com> Cc: Andrey Konovalov <andreyknvl@google.com> Cc: linux-mm@kvack.org --- This patch was previously called "kmsan: call KMSAN hooks where needed" v4: - split this patch away Change-Id: Idd0f8ce858975112285706ffb7286f570bd3007b --- drivers/usb/core/urb.c | 2 ++ 1 file changed, 2 insertions(+)