Skip to contentSkip to conversation

usb: gadget: f_midi2: fix use-after-free in string attribute show path

PATCH In review Ivy Lopez 2 participants 2 messages Aug 16 USB

lore
Share & tools
lore

Ivy Lopez

author
#0 lore raw

PATCH 1/1 usb: gadget: f_midi2: fix use-after-free in string attribute show path

f_midi2_opts_str_show() takes the string lock internally, but its callers dereference the opts->info.<field> pointer before calling it, outside the lock. This races with f_midi2_opts_str_store(), which frees the old string under opts->lock when the attribute is written concurrently, the show path can read a pointer that gets freed before the lock inside str_show() is even taken.

Change f_midi2_opts_str_show() to take a pointer to the string field, matching the existing pattern in f_midi2_opts_str_store(), and dereference it only after the lock is held. Update all three callers (iface_name, block name, and the EP string option macro) accordingly.

Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=2280f1cca5e6b0c353e4
Signed-off-by Ivy Lopez <[email protected]>
---
 drivers/usb/gadget/function/f_midi2.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/usb/gadget/function/f_midi2.c +6 -6
index 19fdac024343..6bfd171b96a9 100644
@@ -2178,13 +2178,13 @@ static ssize_t f_midi2_opts_bool_store(struct f_midi2_opts *opts,
 
 /* generic show/store for string */
 static ssize_t f_midi2_opts_str_show(struct f_midi2_opts *opts,
-				     const char *str, char *page)
+				     const char **strp, char *page)
 {
 	int result = 0;
 
 	mutex_lock(&opts->lock);
-	if (str)
-		result = scnprintf(page, PAGE_SIZE, "%s\n", str);
+	if (*strp)
+		result = scnprintf(page, PAGE_SIZE, "%s\n", *strp);
 	mutex_unlock(&opts->lock);
 	return result;
 }

@@ -2278,7 +2278,7 @@ static ssize_t f_midi2_block_opts_name_show(struct config_item *item,
 {
 	struct f_midi2_block_opts *opts = to_f_midi2_block_opts(item);
 
-	return f_midi2_opts_str_show(opts->ep->opts, opts->info.name, page);
+	return f_midi2_opts_str_show(opts->ep->opts, &opts->info.name, page);
 }
 
 static ssize_t f_midi2_block_opts_name_store(struct config_item *item,

@@ -2435,7 +2435,7 @@ static ssize_t f_midi2_ep_opts_##name##_show(struct config_item *item,	\
 					     char *page)		\
 {									\
 	struct f_midi2_ep_opts *opts = to_f_midi2_ep_opts(item);	\
-	return f_midi2_opts_str_show(opts->opts, opts->info.name, page);\
+	return f_midi2_opts_str_show(opts->opts, &opts->info.name, page);\
 }									\
 									\
 static ssize_t f_midi2_ep_opts_##name##_store(struct config_item *item,	\

@@ -2589,7 +2589,7 @@ static ssize_t f_midi2_opts_iface_name_show(struct config_item *item,
 {
 	struct f_midi2_opts *opts = to_f_midi2_opts(item);
 
-	return f_midi2_opts_str_show(opts, opts->info.iface_name, page);
+	return f_midi2_opts_str_show(opts, &opts->info.iface_name, page);
 }
 
 static ssize_t f_midi2_opts_iface_name_store(struct config_item *item,
Signature · 1 line
2.55.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 8 lines · Ivy Lopez

f_midi2_opts_str_show() takes the string lock internally, but its callers dereference the opts->info.<field> pointer before calling it, outside the lock. This races with f_midi2_opts_str_store(), which frees the old string under opts->lock when the attribute is written concurrently, the show path can read a pointer that gets freed before the lock inside str_show() is even taken.

Quoted 8 lines

Change f_midi2_opts_str_show() to take a pointer to the string field, matching the existing pattern in f_midi2_opts_str_store(), and dereference it only after the lock is held. Update all three callers (iface_name, block name, and the EP string option macro) accordingly.

Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=2280f1cca5e6b0c353e4
Signed-off-by Ivy Lopez <[email protected]>
Reviewed-by Takashi Iwai <[email protected]>

thanks,

Takashi