[PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[Mattijs Korpershoek]

Zixun has reported an odd problem in [1].

He encountered a data abort on the 2nd "ums 0 mmc 0" command with
sam9x60-curiosity board:

U-Boot> ums 0 mmc 0
UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x1d29000
CTRL+C - Operation aborted
U-Boot> ums 0 mmc 0
UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x1d29000
data abort
pc : [<27f93428>]          lr : [<27ef7e80>]
reloc pc : [<23f16428>]    lr : [<23e7ae80>]
sp : 27ef4cf0  ip : a5200000     fp : 23f6915c
r10: deadbeef  r9 : 27ef7e80     r8 : 27f7d2a0
r7 : a5200000  r6 : 00000000     r5 : 00000000  r4 : 27f01668
r3 : 00000000  r2 : 00000000     r1 : 27fe1d88  r0 : 27f01668
Flags: nzCV  IRQs off  FIQs off  Mode SVC_32 (T)
Code: 45ac d017 68c5 4667 (60fd) 60af

I could reproduce this on khadas-vim3_android_ab_defconfig by applying
the following diff:

diff --git a/configs/khadas-vim3_android_ab_defconfig b/configs/khadas-vim3_android_ab_defconfig
index a078c5d363ae..c8d1cc69f1fb 100644
--- a/configs/khadas-vim3_android_ab_defconfig
+++ b/configs/khadas-vim3_android_ab_defconfig
@@ -3,7 +3,7 @@ CONFIG_SYS_BOARD="vim3"
 CONFIG_SYS_CONFIG_NAME="khadas-vim3_android"
 CONFIG_ARCH_MESON=y
 CONFIG_TEXT_BASE=0x01000000
-CONFIG_SYS_MALLOC_LEN=0x08000000
+CONFIG_SYS_MALLOC_LEN=0x81000
 CONFIG_NR_DRAM_BANKS=1
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x20000000

This series fixes the above mentioned crash by introducing proper error
handling and freeing the buffers in the unbind callback.

[1] https://lore.kernel.org/r/all/CA+GyqebHib_N7szGKkR0Ejac1rmoDQp+0a8t7ROng=PE3g9pGA@mail.gmail.com/

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
This should go into next since it's not an urgent fix.
---

---
Mattijs Korpershoek (4):
      usb: gadget: f_mass_storage: Remove kref structure use
      usb: gadget: f_mass_storage: Drop invalid kfree() in fsg_common_release()
      usb: gadget: f_mass_storage: Fix NULL dereference in fsg_add()
      usb: gadget: f_mass_storage: Fix memory leak of fsg buffers

 drivers/usb/gadget/f_mass_storage.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)
---
base-commit: 3f9e0349c3e8afe45d5bdb2328a16512cfbc2ef1
change-id: 20250327-ums-gadget-leak-898d2e776e2d

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek@baylibre.com>

[PATCH 1/4] usb: gadget: f_mass_storage: Remove kref structure use

[Mattijs Korpershoek]

The kref structure is locally to f_mass_storage and is not used
anywhere beside in fsg_common_release().

Remove it and use struct fsg_common* instead.

No functional change.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/usb/gadget/f_mass_storage.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index d3fc4acb401a..bd749c033f9a 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -284,7 +284,6 @@ static const char fsg_string_interface[] = "Mass Storage";
 #define kthread_create(...)	__builtin_return_address(0)
 #define wait_for_completion(...) do {} while (0)
 
-struct kref {int x; };
 struct completion {int x; };
 
 struct fsg_dev;
@@ -345,8 +344,6 @@ struct fsg_common {
 	/* Vendor (8 chars), product (16 chars), release (4
 	 * hexadecimal digits) and NUL byte */
 	char inquiry_string[8 + 16 + 4 + 1];
-
-	struct kref		ref;
 };
 
 struct fsg_config {
@@ -2436,7 +2433,7 @@ int fsg_main_thread(void *common_)
 	return 0;
 }
 
-static void fsg_common_release(struct kref *ref);
+static void fsg_common_release(struct fsg_common *common);
 
 static struct fsg_common *fsg_common_init(struct fsg_common *common,
 					  struct usb_composite_dev *cdev)
@@ -2548,16 +2545,12 @@ error_luns:
 	common->nluns = i + 1;
 error_release:
 	common->state = FSG_STATE_TERMINATED;	/* The thread is dead */
-	/* Call fsg_common_release() directly, ref might be not
-	 * initialised */
-	fsg_common_release(&common->ref);
+	fsg_common_release(common);
 	return ERR_PTR(rc);
 }
 
-static void fsg_common_release(struct kref *ref)
+static void fsg_common_release(struct fsg_common *common)
 {
-	struct fsg_common *common = container_of(ref, struct fsg_common, ref);
-
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
 		raise_exception(common, FSG_STATE_EXIT);

-- 
2.49.0

[PATCH 2/4] usb: gadget: f_mass_storage: Drop invalid kfree() in fsg_common_release()

[Mattijs Korpershoek]

Boards with low memory (CONFIG_SYS_MALLOC_LEN=0x81000), can be crashed
using the => ums command twice in row:

=> ums 0 mmc 2
UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000
|crq->brequest:0x0
CTRL+C - Operation aborted
=> ums 0 mmc 2
UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000
"Synchronous Abort" handler, esr 0x96000004, far 0xfffffffff2ea20f0
elr: 000000000102ea78 lr : 000000000105e028 (reloc)
elr: 00000000f2f33a78 lr : 00000000f2f63028
x0 : 0000000100000000 x1 : 0000000100000000
x2 : 0000000000000000 x3 : fffffffff2ea20e0
x4 : 00000000f2fc9720 x5 : 00000000f2ea20e0
x6 : 00000000f2fc9730 x7 : 00000000f2ee4780
x8 : 000000000000003f x9 : 0000000000000004
x10: 0000000000000058 x11: 00000000000058c4
x12: 0000000000000000 x13: 00000000f2e60800
x14: 00000000f4ec0040 x15: 0000000000000000
x16: 00000000f2f62f2c x17: 0000000000c0c0c0
x18: 00000000f2e73e00 x19: 00000000f2ea2010
x20: 00000000fffffff4 x21: 00000000f2e9b500
x22: 00000000f2ea20f0 x23: 00000000f2ea2050
x24: 00000000f2f61eec x25: 00000000f2fcf000
x26: 00000000f2e9fcd0 x27: 0000000000000000
x28: 0000000000000000 x29: 00000000f2e60290

Code: d00004a6 911cc0c6 cb000063 8b000021 (f9400860)
Resetting CPU ...

This happens when fsg_common_init() fails to allocate memory and calls
fsg_common_release().
fsg_common_release() then calls kfree() which frees common->luns.
However, common->luns was never allocated via kmalloc/calloc(),
resulting in a crash.

Drop the invalid kfree. The memory from common->luns will be
reclaimed when we kfree(common) later in fgs_common_release().

Reported-by: Zixun LI <admin@hifiphile.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/usb/gadget/f_mass_storage.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index bd749c033f9a..6f464185bd39 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2564,8 +2564,6 @@ static void fsg_common_release(struct fsg_common *common)
 		/* In error recovery common->nluns may be zero. */
 		for (; i; --i, ++lun)
 			fsg_lun_close(lun);
-
-		kfree(common->luns);
 	}
 
 	{

-- 
2.49.0

[PATCH 3/4] usb: gadget: f_mass_storage: Fix NULL dereference in fsg_add()

[Mattijs Korpershoek]

fsg_common_init() can fail when memory is low. In that case, it returns
PTR_ERR().
fsg_add() does not check for failure, and thus dereferences an invalid
fsg_common later, which crashes.

Verify if we receive an error from fsg_common_init() and handle it
gracefully.

Reported-by: Zixun LI <admin@hifiphile.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/usb/gadget/f_mass_storage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 6f464185bd39..fcce6d12f56b 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2742,6 +2742,8 @@ int fsg_add(struct usb_configuration *c)
 	struct fsg_common *fsg_common;
 
 	fsg_common = fsg_common_init(NULL, c->cdev);
+	if (IS_ERR(fsg_common))
+		return PTR_ERR(fsg_common);
 
 	fsg_common->vendor_name = 0;
 	fsg_common->product_name = 0;

-- 
2.49.0

[PATCH 4/4] usb: gadget: f_mass_storage: Fix memory leak of fsg buffers

[Mattijs Korpershoek]

In fsg_common_init, we allocate some buffers via memalign().
However, these buffers are never freed.

Because of that, we cannot call => ums command multiple times on boards
with low memory (CONFIG_SYS_MALLOC_LEN=0x81000):

=> ums 0 mmc 2
UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000
|crq->brequest:0x0
CTRL+C - Operation aborted
=> ums 0 mmc 2
UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000
failed to start <NULL>: -12
g_dnl_register: failed!, error: -12
g_dnl_register failed

Make sure the fsg buffers are freed when the gadget is unbound by
calling fsg_common_release() in fsg_unbind().

Reported-by: Zixun LI <admin@hifiphile.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/usb/gadget/f_mass_storage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index fcce6d12f56b..71dc58da3f03 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2639,6 +2639,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 		raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
 	}
 
+	fsg_common_release(fsg->common);
 	free(fsg->function.descriptors);
 	free(fsg->function.hs_descriptors);
 	kfree(fsg);

-- 
2.49.0

Re: [PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[Zixun LI]

On Fri, Mar 28, 2025 at 9:15 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
> This series fixes the above mentioned crash by introducing proper error
> handling and freeing the buffers in the unbind callback.

Hi Mattijs,

Thank you for the quick fix, I've tested on SAM9X60 with multiple ums
commands and it works.

Zixun

Re: [PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[Mattijs Korpershoek]

Hi Zixun,

On ven., mars 28, 2025 at 18:38, Zixun LI <admin@hifiphile.com> wrote:

> On Fri, Mar 28, 2025 at 9:15 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>> This series fixes the above mentioned crash by introducing proper error
>> handling and freeing the buffers in the unbind callback.
>
> Hi Mattijs,
>
> Thank you for the quick fix, I've tested on SAM9X60 with multiple ums
> commands and it works.

Thank you for testing. Testing on hardware is very valuable to the
U-Boot community.
It is so valuable that we give credit in the commit history for testing.

In order to give you proper credit for testing, a special email line is
expected to be found:

"""
Tested-by: Zixun LI <admin@hifiphile.com>
"""

You can also specify on which board you've tested:
"""
Tested-by: Zixun LI <admin@hifiphile.com> # on SAM9X60
"""

This allows tools to automatically pick up your testing efforts and put
them in the commit log.

See: https://docs.u-boot.org/en/latest/develop/process.html#review-process-git-tags

If you are OK with being mentioned in the commit log, could you please
answer this email thread with the special "Tested-by:" line.

Also, do not hesitate to use that in the future.

Thanks again for contributing to U-Boot!
Mattijs

>
> Zixun

Re: [PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[Zixun LI]

On Sat, Mar 29, 2025 at 11:04 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> If you are OK with being mentioned in the commit log, could you please
> answer this email thread with the special "Tested-by:" line.
>
> Mattijs

Hi Mattijs,

Yes of course, I searched a bit for tag usage but did end up with the right
document you linked ;)

Tested-by: Zixun LI <admin@hifiphile.com> # on SAM9X60

Zixun

Re: [PATCH 0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices

[Mattijs Korpershoek]

Hi,

On Fri, 28 Mar 2025 09:15:40 +0100, Mattijs Korpershoek wrote:
> Zixun has reported an odd problem in [1].
> 
> He encountered a data abort on the 2nd "ums 0 mmc 0" command with
> sam9x60-curiosity board:
> 
> U-Boot> ums 0 mmc 0
> UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x1d29000
> CTRL+C - Operation aborted
> U-Boot> ums 0 mmc 0
> UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x1d29000
> data abort
> pc : [<27f93428>]          lr : [<27ef7e80>]
> reloc pc : [<23f16428>]    lr : [<23e7ae80>]
> sp : 27ef4cf0  ip : a5200000     fp : 23f6915c
> r10: deadbeef  r9 : 27ef7e80     r8 : 27f7d2a0
> r7 : a5200000  r6 : 00000000     r5 : 00000000  r4 : 27f01668
> r3 : 00000000  r2 : 00000000     r1 : 27fe1d88  r0 : 27f01668
> Flags: nzCV  IRQs off  FIQs off  Mode SVC_32 (T)
> Code: 45ac d017 68c5 4667 (60fd) 60af
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[0/4] usb: gadget: f_mass_storage: Fix crashes on low memory devices
      (no commit info)
[1/4] usb: gadget: f_mass_storage: Remove kref structure use
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/a4317be9dbe9428f6209cb5595bfc57eb990b023
[2/4] usb: gadget: f_mass_storage: Drop invalid kfree() in fsg_common_release()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/47fd46db943a55a7e0232e27224d216b774d7575
[3/4] usb: gadget: f_mass_storage: Fix NULL dereference in fsg_add()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/6c9eaec55ae634bac02e0cf70eb78c879d008a5c
[4/4] usb: gadget: f_mass_storage: Fix memory leak of fsg buffers
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/c76a7090f6480affbe3946fcd0dcc6df9bfe05b3

--
Mattijs