* [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free @ 2017-01-17 9:59 Jim Lin 2017-01-17 10:29 ` Felipe Balbi 2021-03-11 6:42 ` [PATCH v4] " Macpaul Lin 0 siblings, 2 replies; 7+ messages in thread From: Jim Lin @ 2017-01-17 9:59 UTC (permalink / raw) To: balbi; +Cc: linux-usb, linux-kernel, Jim Lin When gadget is disconnected, running sequence is like this. . composite_disconnect . Call trace: usb_string_copy+0xd0/0x128 gadget_config_name_configuration_store+0x4 gadget_config_name_attr_store+0x40/0x50 configfs_write_file+0x198/0x1f4 vfs_write+0x100/0x220 SyS_write+0x58/0xa8 . configfs_composite_unbind . configfs_composite_bind In configfs_composite_bind, it has "cn->strings.s = cn->configuration;" When usb_string_copy is invoked. it would allocate memory, copy input string, release previous pointed memory space, and use new allocated memory. When gadget is connected, host sends down request to get information. Call trace: usb_gadget_get_string+0xec/0x168 lookup_string+0x64/0x98 composite_setup+0xa34/0x1ee8 If gadget is disconnected and connected quickly, in the failed case, cn->configuration memory has been released by usb_string_copy kfree but configfs_composite_bind hasn't been run in time to assign new allocated "cn->configuration" pointer to "cn->strings.s". When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling memory is accessed, "BUG: KASAN: use-after-free" error occurs. Signed-off-by: Jim Lin <jilin@nvidia.com> --- Changes in v2: Changes in v3: Change commit description drivers/usb/gadget/configfs.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c index 78c4497..39fea62 100644 --- a/drivers/usb/gadget/configfs.c +++ b/drivers/usb/gadget/configfs.c @@ -106,6 +106,9 @@ struct gadget_config_name { struct list_head list; }; +#define MAX_USB_STRING_LEN 126 +#define MAX_USB_STRING_WITH_NULL_LEN (MAX_USB_STRING_LEN+1) + static int usb_string_copy(const char *s, char **s_copy) { int ret; @@ -115,12 +118,16 @@ static int usb_string_copy(const char *s, char **s_copy) if (ret > 126) return -EOVERFLOW; - str = kstrdup(s, GFP_KERNEL); - if (!str) - return -ENOMEM; + if (copy) { + str = copy; + } else { + str = kmalloc(MAX_USB_STRING_WITH_NULL_LEN, GFP_KERNEL); + if (!str) + return -ENOMEM; + } + strcpy(str, s); if (str[ret - 1] == '\n') str[ret - 1] = '\0'; - kfree(copy); *s_copy = str; return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free 2017-01-17 9:59 [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free Jim Lin @ 2017-01-17 10:29 ` Felipe Balbi 2017-02-10 6:44 ` Macpaul Lin 2021-02-22 23:47 ` Thadeu Lima de Souza Cascardo 2021-03-11 6:42 ` [PATCH v4] " Macpaul Lin 1 sibling, 2 replies; 7+ messages in thread From: Felipe Balbi @ 2017-01-17 10:29 UTC (permalink / raw) To: Jim Lin; +Cc: linux-usb, linux-kernel, Jim Lin [-- Attachment #1: Type: text/plain, Size: 1688 bytes --] Hi, Jim Lin <jilin@nvidia.com> writes: > When gadget is disconnected, running sequence is like this. > . composite_disconnect > . Call trace: > usb_string_copy+0xd0/0x128 > gadget_config_name_configuration_store+0x4 > gadget_config_name_attr_store+0x40/0x50 > configfs_write_file+0x198/0x1f4 > vfs_write+0x100/0x220 > SyS_write+0x58/0xa8 > . configfs_composite_unbind > . configfs_composite_bind > > In configfs_composite_bind, it has > "cn->strings.s = cn->configuration;" > > When usb_string_copy is invoked. it would > allocate memory, copy input string, release previous pointed memory space, > and use new allocated memory. > > When gadget is connected, host sends down request to get information. > Call trace: > usb_gadget_get_string+0xec/0x168 > lookup_string+0x64/0x98 > composite_setup+0xa34/0x1ee8 > > If gadget is disconnected and connected quickly, in the failed case, > cn->configuration memory has been released by usb_string_copy kfree but > configfs_composite_bind hasn't been run in time to assign new allocated > "cn->configuration" pointer to "cn->strings.s". > > When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling > memory is accessed, "BUG: KASAN: use-after-free" error occurs. > > Signed-off-by: Jim Lin <jilin@nvidia.com> > --- > Changes in v2: > Changes in v3: > Change commit description well, I need to be sure you tested this with Linus' tree. The reason I'm asking is because this could be a bug caused by Android changes. From your previous patch, the problem started with android_setup(). Please test with v4.10-rc4 and any configfs-based gadget. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free 2017-01-17 10:29 ` Felipe Balbi @ 2017-02-10 6:44 ` Macpaul Lin 2021-02-22 23:47 ` Thadeu Lima de Souza Cascardo 1 sibling, 0 replies; 7+ messages in thread From: Macpaul Lin @ 2017-02-10 6:44 UTC (permalink / raw) To: Felipe Balbi; +Cc: Jim Lin, linux-usb, linux-kernel Hi Jim, > Jim Lin <jilin@nvidia.com> writes: > > When gadget is disconnected, running sequence is like this. > > . composite_disconnect > > . Call trace: > > usb_string_copy+0xd0/0x128 > > gadget_config_name_configuration_store+0x4 > > gadget_config_name_attr_store+0x40/0x50 > > configfs_write_file+0x198/0x1f4 > > vfs_write+0x100/0x220 > > SyS_write+0x58/0xa8 > > . configfs_composite_unbind > > . configfs_composite_bind > > [deleted] > > When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling > > memory is accessed, "BUG: KASAN: use-after-free" error occurs. > > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > --- > > Changes in v2: > > Changes in v3: > > Change commit description > > well, I need to be sure you tested this with Linus' tree. The reason I'm > asking is because this could be a bug caused by Android changes. From > your previous patch, the problem started with android_setup(). > > Please test with v4.10-rc4 and any configfs-based gadget. > > -- > balbi I've got the similar problem on Android, however, Linux guys require you and other people to test your patch on pure Linux. Since Linux is exactly a "PC" based OS, only common patches should be commit to Linux code base. Except the bug is quite common in 3 OS, in "Linux PC" and in "Android Linux" or "Chromium OS". I'm not sure about the difference between Chromium OS and Linux PC. According to CVE report, it looks like the change is from Chromium OS? Dose Nvidia has a pure Linux software team can verify your patch on your platform? I think if you can prove the result is okay on Linux PC or on Chromium OS will help. -- Best regards, Macpaul Lin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free 2017-01-17 10:29 ` Felipe Balbi 2017-02-10 6:44 ` Macpaul Lin @ 2021-02-22 23:47 ` Thadeu Lima de Souza Cascardo 1 sibling, 0 replies; 7+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2021-02-22 23:47 UTC (permalink / raw) To: Felipe Balbi; +Cc: Jim Lin, linux-usb, linux-kernel On Tue, Jan 17, 2017 at 12:29:09PM +0200, Felipe Balbi wrote: > > Hi, > > Jim Lin <jilin@nvidia.com> writes: > > When gadget is disconnected, running sequence is like this. > > . composite_disconnect > > . Call trace: > > usb_string_copy+0xd0/0x128 > > gadget_config_name_configuration_store+0x4 > > gadget_config_name_attr_store+0x40/0x50 > > configfs_write_file+0x198/0x1f4 > > vfs_write+0x100/0x220 > > SyS_write+0x58/0xa8 > > . configfs_composite_unbind > > . configfs_composite_bind > > > > In configfs_composite_bind, it has > > "cn->strings.s = cn->configuration;" > > > > When usb_string_copy is invoked. it would > > allocate memory, copy input string, release previous pointed memory space, > > and use new allocated memory. > > > > When gadget is connected, host sends down request to get information. > > Call trace: > > usb_gadget_get_string+0xec/0x168 > > lookup_string+0x64/0x98 > > composite_setup+0xa34/0x1ee8 > > > > If gadget is disconnected and connected quickly, in the failed case, > > cn->configuration memory has been released by usb_string_copy kfree but > > configfs_composite_bind hasn't been run in time to assign new allocated > > "cn->configuration" pointer to "cn->strings.s". > > > > When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling > > memory is accessed, "BUG: KASAN: use-after-free" error occurs. > > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > --- > > Changes in v2: > > Changes in v3: > > Change commit description > > well, I need to be sure you tested this with Linus' tree. The reason I'm > asking is because this could be a bug caused by Android changes. From > your previous patch, the problem started with android_setup(). > > Please test with v4.10-rc4 and any configfs-based gadget. > > -- > balbi I tested this with dummy_hcd on top of a 5.8 kernel and I got lsusb to respond with an error instead of the right manufacturer string, after overwriting such a string after binding. With the patch applied, after the string is overwritten, lsusb will show the updated string. Because of commit 81c7462883b0cc0a4eeef0687f80ad5b5baee5f6 ("USB: replace hardcode maximum usb string length by definition"), the patch will need a fixup. Should I send a v2 with my sign-off? Thanks. Cascardo. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4] usb: gadget: configfs: Fix KASAN use-after-free 2017-01-17 9:59 [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free Jim Lin 2017-01-17 10:29 ` Felipe Balbi @ 2021-03-11 6:42 ` Macpaul Lin 2021-03-11 6:53 ` Macpaul Lin 1 sibling, 1 reply; 7+ messages in thread From: Macpaul Lin @ 2021-03-11 6:42 UTC (permalink / raw) To: Jim Lin, Thadeu Lima de Souza Cascardo, Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger, linux-usb, linux-kernel, linux-arm-kernel, linux-mediatek Cc: Ainge Hsu, Eddie Hung, Kuohong Wang, Mediatek WSD Upstream, Macpaul Lin, Macpaul Lin, stable From: Jim Lin <jilin@nvidia.com> When gadget is disconnected, running sequence is like this. . composite_disconnect . Call trace: usb_string_copy+0xd0/0x128 gadget_config_name_configuration_store+0x4 gadget_config_name_attr_store+0x40/0x50 configfs_write_file+0x198/0x1f4 vfs_write+0x100/0x220 SyS_write+0x58/0xa8 . configfs_composite_unbind . configfs_composite_bind In configfs_composite_bind, it has "cn->strings.s = cn->configuration;" When usb_string_copy is invoked. it would allocate memory, copy input string, release previous pointed memory space, and use new allocated memory. When gadget is connected, host sends down request to get information. Call trace: usb_gadget_get_string+0xec/0x168 lookup_string+0x64/0x98 composite_setup+0xa34/0x1ee8 If gadget is disconnected and connected quickly, in the failed case, cn->configuration memory has been released by usb_string_copy kfree but configfs_composite_bind hasn't been run in time to assign new allocated "cn->configuration" pointer to "cn->strings.s". When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling memory is accessed, "BUG: KASAN: use-after-free" error occurs. Signed-off-by: Jim Lin <jilin@nvidia.com> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com> Cc: stable@vger.kernel.org --- Changes in v2: Changes in v3: - Change commit description Changes in v4: - Fix build error and adapt patch to kernel-5.12-rc1. Replace definition "MAX_USB_STRING_WITH_NULL_LEN" with "USB_MAX_STRING_WITH_NULL_LEN". - Note: The patch v2 and v3 has been verified by Thadeu Lima de Souza Cascardo <cascardo@canonical.com> http://spinics.net/lists/kernel/msg3840792.html and Macpaul Lin <macpaul.lin@mediatek.com> on Android kernels. http://lkml.org/lkml/2020/6/11/8 - The patch is suggested to be applied to LTS versions. drivers/usb/gadget/configfs.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c index 0d56f33..15a607c 100644 --- a/drivers/usb/gadget/configfs.c +++ b/drivers/usb/gadget/configfs.c @@ -97,6 +97,8 @@ struct gadget_config_name { struct list_head list; }; +#define USB_MAX_STRING_WITH_NULL_LEN (USB_MAX_STRING_LEN+1) + static int usb_string_copy(const char *s, char **s_copy) { int ret; @@ -106,12 +108,16 @@ static int usb_string_copy(const char *s, char **s_copy) if (ret > USB_MAX_STRING_LEN) return -EOVERFLOW; - str = kstrdup(s, GFP_KERNEL); - if (!str) - return -ENOMEM; + if (copy) { + str = copy; + } else { + str = kmalloc(USB_MAX_STRING_WITH_NULL_LEN, GFP_KERNEL); + if (!str) + return -ENOMEM; + } + strcpy(str, s); if (str[ret - 1] == '\n') str[ret - 1] = '\0'; - kfree(copy); *s_copy = str; return 0; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] usb: gadget: configfs: Fix KASAN use-after-free 2021-03-11 6:42 ` [PATCH v4] " Macpaul Lin @ 2021-03-11 6:53 ` Macpaul Lin 2021-03-11 10:52 ` Thadeu Lima de Souza Cascardo 0 siblings, 1 reply; 7+ messages in thread From: Macpaul Lin @ 2021-03-11 6:53 UTC (permalink / raw) To: Jim Lin, Thadeu Lima de Souza Cascardo Cc: Thadeu Lima de Souza Cascardo, Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger, linux-usb, linux-kernel, linux-arm-kernel, linux-mediatek, Ainge Hsu, Eddie Hung, Kuohong Wang, Mediatek WSD Upstream, Macpaul Lin, stable On Thu, 2021-03-11 at 14:42 +0800, Macpaul Lin wrote: > From: Jim Lin <jilin@nvidia.com> > > When gadget is disconnected, running sequence is like this. > . composite_disconnect > . Call trace: > usb_string_copy+0xd0/0x128 > gadget_config_name_configuration_store+0x4 > gadget_config_name_attr_store+0x40/0x50 > configfs_write_file+0x198/0x1f4 > vfs_write+0x100/0x220 > SyS_write+0x58/0xa8 > . configfs_composite_unbind > . configfs_composite_bind > > In configfs_composite_bind, it has > "cn->strings.s = cn->configuration;" > > When usb_string_copy is invoked. it would > allocate memory, copy input string, release previous pointed memory space, > and use new allocated memory. > > When gadget is connected, host sends down request to get information. > Call trace: > usb_gadget_get_string+0xec/0x168 > lookup_string+0x64/0x98 > composite_setup+0xa34/0x1ee8 > > If gadget is disconnected and connected quickly, in the failed case, > cn->configuration memory has been released by usb_string_copy kfree but > configfs_composite_bind hasn't been run in time to assign new allocated > "cn->configuration" pointer to "cn->strings.s". > > When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling > memory is accessed, "BUG: KASAN: use-after-free" error occurs. > > Signed-off-by: Jim Lin <jilin@nvidia.com> > Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com> > Cc: stable@vger.kernel.org > --- > Changes in v2: > Changes in v3: > - Change commit description > Changes in v4: > - Fix build error and adapt patch to kernel-5.12-rc1. > Replace definition "MAX_USB_STRING_WITH_NULL_LEN" with > "USB_MAX_STRING_WITH_NULL_LEN". > - Note: The patch v2 and v3 has been verified by > Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > http://spinics.net/lists/kernel/msg3840792.html Dear Cascardo, Would you please help to confirm if you've tested it on Linux PC, Chrome OS, or an Android OS? Thanks! Macpaul Lin > and > Macpaul Lin <macpaul.lin@mediatek.com> on Android kernels. > http://lkml.org/lkml/2020/6/11/8 > - The patch is suggested to be applied to LTS versions. > > drivers/usb/gadget/configfs.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c > index 0d56f33..15a607c 100644 > --- a/drivers/usb/gadget/configfs.c > +++ b/drivers/usb/gadget/configfs.c > @@ -97,6 +97,8 @@ struct gadget_config_name { > struct list_head list; > }; > > +#define USB_MAX_STRING_WITH_NULL_LEN (USB_MAX_STRING_LEN+1) > + > static int usb_string_copy(const char *s, char **s_copy) > { > int ret; > @@ -106,12 +108,16 @@ static int usb_string_copy(const char *s, char **s_copy) > if (ret > USB_MAX_STRING_LEN) > return -EOVERFLOW; > > - str = kstrdup(s, GFP_KERNEL); > - if (!str) > - return -ENOMEM; > + if (copy) { > + str = copy; > + } else { > + str = kmalloc(USB_MAX_STRING_WITH_NULL_LEN, GFP_KERNEL); > + if (!str) > + return -ENOMEM; > + } > + strcpy(str, s); > if (str[ret - 1] == '\n') > str[ret - 1] = '\0'; > - kfree(copy); > *s_copy = str; > return 0; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] usb: gadget: configfs: Fix KASAN use-after-free 2021-03-11 6:53 ` Macpaul Lin @ 2021-03-11 10:52 ` Thadeu Lima de Souza Cascardo 0 siblings, 0 replies; 7+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2021-03-11 10:52 UTC (permalink / raw) To: Macpaul Lin Cc: Jim Lin, Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger, linux-usb, linux-kernel, linux-arm-kernel, linux-mediatek, Ainge Hsu, Eddie Hung, Kuohong Wang, Mediatek WSD Upstream, Macpaul Lin, stable On Thu, Mar 11, 2021 at 02:53:52PM +0800, Macpaul Lin wrote: > On Thu, 2021-03-11 at 14:42 +0800, Macpaul Lin wrote: > > From: Jim Lin <jilin@nvidia.com> > > > > When gadget is disconnected, running sequence is like this. > > . composite_disconnect > > . Call trace: > > usb_string_copy+0xd0/0x128 > > gadget_config_name_configuration_store+0x4 > > gadget_config_name_attr_store+0x40/0x50 > > configfs_write_file+0x198/0x1f4 > > vfs_write+0x100/0x220 > > SyS_write+0x58/0xa8 > > . configfs_composite_unbind > > . configfs_composite_bind > > > > In configfs_composite_bind, it has > > "cn->strings.s = cn->configuration;" > > > > When usb_string_copy is invoked. it would > > allocate memory, copy input string, release previous pointed memory space, > > and use new allocated memory. > > > > When gadget is connected, host sends down request to get information. > > Call trace: > > usb_gadget_get_string+0xec/0x168 > > lookup_string+0x64/0x98 > > composite_setup+0xa34/0x1ee8 > > > > If gadget is disconnected and connected quickly, in the failed case, > > cn->configuration memory has been released by usb_string_copy kfree but > > configfs_composite_bind hasn't been run in time to assign new allocated > > "cn->configuration" pointer to "cn->strings.s". > > > > When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling > > memory is accessed, "BUG: KASAN: use-after-free" error occurs. > > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com> > > Cc: stable@vger.kernel.org > > --- > > Changes in v2: > > Changes in v3: > > - Change commit description > > Changes in v4: > > - Fix build error and adapt patch to kernel-5.12-rc1. > > Replace definition "MAX_USB_STRING_WITH_NULL_LEN" with > > "USB_MAX_STRING_WITH_NULL_LEN". > > - Note: The patch v2 and v3 has been verified by > > Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > http://spinics.net/lists/kernel/msg3840792.html > > Dear Cascardo, > > Would you please help to confirm if you've tested it on Linux PC, > Chrome OS, or an Android OS? I tested v3 on Ubuntu GNU/Linux. I will test v4. Cascardo. > > Thanks! > Macpaul Lin > > > and > > Macpaul Lin <macpaul.lin@mediatek.com> on Android kernels. > > http://lkml.org/lkml/2020/6/11/8 > > - The patch is suggested to be applied to LTS versions. > > > > drivers/usb/gadget/configfs.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c > > index 0d56f33..15a607c 100644 > > --- a/drivers/usb/gadget/configfs.c > > +++ b/drivers/usb/gadget/configfs.c > > @@ -97,6 +97,8 @@ struct gadget_config_name { > > struct list_head list; > > }; > > > > +#define USB_MAX_STRING_WITH_NULL_LEN (USB_MAX_STRING_LEN+1) > > + > > static int usb_string_copy(const char *s, char **s_copy) > > { > > int ret; > > @@ -106,12 +108,16 @@ static int usb_string_copy(const char *s, char **s_copy) > > if (ret > USB_MAX_STRING_LEN) > > return -EOVERFLOW; > > > > - str = kstrdup(s, GFP_KERNEL); > > - if (!str) > > - return -ENOMEM; > > + if (copy) { > > + str = copy; > > + } else { > > + str = kmalloc(USB_MAX_STRING_WITH_NULL_LEN, GFP_KERNEL); > > + if (!str) > > + return -ENOMEM; > > + } > > + strcpy(str, s); > > if (str[ret - 1] == '\n') > > str[ret - 1] = '\0'; > > - kfree(copy); > > *s_copy = str; > > return 0; > > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-11 10:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-17 9:59 [PATCH v3] usb: gadget: configfs: Fix KASAN use-after-free Jim Lin 2017-01-17 10:29 ` Felipe Balbi 2017-02-10 6:44 ` Macpaul Lin 2021-02-22 23:47 ` Thadeu Lima de Souza Cascardo 2021-03-11 6:42 ` [PATCH v4] " Macpaul Lin 2021-03-11 6:53 ` Macpaul Lin 2021-03-11 10:52 ` Thadeu Lima de Souza Cascardo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).