usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer

A reference to struct f_hidg is shared with this driver's associated character device handling component without provision for life-time handling. In some circumstances, this can lead to unwanted behaviour depending on the order in which things are torn down.

Utilise, the reference counting functionality already provided by the implanted character device structure to ensure the struct f_hidg's memory is only freed once the character device handling has finished with it.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

Quoted 10 lines · Lee Jones

A reference to struct f_hidg is shared with this driver's associated character device handling component without provision for life-time handling. In some circumstances, this can lead to unwanted behaviour depending on the order in which things are torn down.

Utilise, the reference counting functionality already provided by the implanted character device structure to ensure the struct f_hidg's memory is only freed once the character device handling has finished with it.

Quoted 4 lines

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

Quoted 40 lines · Greg KH

Quoted 10 lines · Lee Jones

A reference to struct f_hidg is shared with this driver's associated character device handling component without provision for life-time handling. In some circumstances, this can lead to unwanted behaviour depending on the order in which things are torn down.

Utilise, the reference counting functionality already provided by the implanted character device structure to ensure the struct f_hidg's memory is only freed once the character device handling has finished with it.

Quoted 4 lines

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

Quoted 3 lines

thanks,

greg k-h's patch email bot

This is a completely new solution to the same problem.

I'm treating this as a brand new submission.

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 92

Quoted 4 lines · Lee Jones

drivers/usb/gadget/function/f_hid.c +4 -0

+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}

Ick, sorry, no, that's not going to work and is not allowed at all. That's some major layering violations there, AND it can change after you get the value as well.

Quoted 9 lines · Greg KH

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 92

Quoted 4 lines · Lee Jones

drivers/usb/gadget/function/f_hid.c +4 -0

+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}

Ick, sorry, no, that's not going to work and is not allowed at all. That's some major layering violations there, AND it can change after you get the value as well.

This cdev belongs solely to this driver. Hence the *.*.* and not *->*->*. What is preventing us from reading our own data? If we cannot do this directly, can I create an API to do it 'officially'?

I do, however, appreciate that a little locking wouldn't go amiss.

If this solution is not acceptable either, then we're left up the creak without a paddle. The rules you've communicated are not compatible with each other.

Rule 1: Only one item in a data structure can reference count.

Due to the embedded cdev struct, this rules out my first solution of giving f_hidg its own kref so that it can conduct its own life-time management.

A potential option to satisfy this rule would be to remove the cdev attribute and create its data dynamically instead. However, the staticness of cdev is used to obtain f_hidg (with container_of()) in the character device handling component, so it cannot be removed.

Rule 2: Reading the present refcount causes a laying violation

So we're essentially saying, if data is already being reference counted, you have to use the present implementation instead of adding additional counts, but there is no way to actually do that, which kind of puts us at stalemate.

Since this is a genuine issue, doing anything is not really an option either. So where do we go from here?

Signature · 1 line

Lee Jones [李琼斯]

Quoted 28 lines · Lee Jones

Quoted 9 lines · Greg KH

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 92

Quoted 4 lines · Lee Jones

drivers/usb/gadget/function/f_hid.c +4 -0

+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}

Ick, sorry, no, that's not going to work and is not allowed at all. That's some major layering violations there, AND it can change after you get the value as well.

This cdev belongs solely to this driver. Hence the *.*.* and not *->*->*. What is preventing us from reading our own data? If we cannot do this directly, can I create an API to do it 'officially'?

I do, however, appreciate that a little locking wouldn't go amiss.

If this solution is not acceptable either, then we're left up the creak without a paddle. The rules you've communicated are not compatible with each other.

Rule 1: Only one item in a data structure can reference count.

Due to the embedded cdev struct, this rules out my first solution of giving f_hidg its own kref so that it can conduct its own life-time management.

Quoted 4 lines

A potential option to satisfy this rule would be to remove the cdev attribute and create its data dynamically instead. However, the staticness of cdev is used to obtain f_hidg (with container_of()) in the character device handling component, so it cannot be removed.

You have not understood this rule correctly. Only one item in a data structure can hold a reference count _for that structure_. But several items in a structure can hold reference counts for themselves.

So for example, you could put a kref in f_hidg which would hold the reference count for the f_hidg structure, while at the same time including an embedded cdev with its own reference counter. The point is that the refcount in the embedded cdev refers to the lifetime of the cdev, not the lifetime of the f_hidg.

To make this work properly, you have to do two additional things:

When the cdev's refcount is initialized, increment the kref in f_hidg.

When the cdev's refcount drops to 0, decrement the kref (and release f_hidg if the kref hits 0).

Quoted 29 lines · Alan Stern

Quoted 28 lines · Lee Jones

Quoted 9 lines · Greg KH

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 92

Quoted 4 lines · Lee Jones

drivers/usb/gadget/function/f_hid.c +4 -0

+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}

Ick, sorry, no, that's not going to work and is not allowed at all. That's some major layering violations there, AND it can change after you get the value as well.

This cdev belongs solely to this driver. Hence the *.*.* and not *->*->*. What is preventing us from reading our own data? If we cannot do this directly, can I create an API to do it 'officially'?

I do, however, appreciate that a little locking wouldn't go amiss.

If this solution is not acceptable either, then we're left up the creak without a paddle. The rules you've communicated are not compatible with each other.

Rule 1: Only one item in a data structure can reference count.

Due to the embedded cdev struct, this rules out my first solution of giving f_hidg its own kref so that it can conduct its own life-time management.

Quoted 8 lines

Quoted 4 lines

A potential option to satisfy this rule would be to remove the cdev attribute and create its data dynamically instead. However, the staticness of cdev is used to obtain f_hidg (with container_of()) in the character device handling component, so it cannot be removed.

You have not understood this rule correctly. Only one item in a data structure can hold a reference count _for that structure_. But several items in a structure can hold reference counts for themselves.

Here was the review comment I was working to on this patch [0]:

"While at first glance, it seems that f_hidg is not reference
counted, it really is, with the embedded "struct cdev" a few lines above this.

That is the reference count that should control the lifecycle of this object, not another reference here in the "outer layer" structure."

Quoted 5 lines

So for example, you could put a kref in f_hidg which would hold the reference count for the f_hidg structure, while at the same time including an embedded cdev with its own reference counter. The point is that the refcount in the embedded cdev refers to the lifetime of the cdev, not the lifetime of the f_hidg.

This was the approach in the original submission [1], which during review I was told was unacceptable for the aforementioned reason.

[0] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/all/[email protected]/

Quoted 2 lines

To make this work properly, you have to do two additional things:

Quoted 5 lines

When the cdev's refcount is initialized, increment the kref in f_hidg.

When the cdev's refcount drops to 0, decrement the kref (and release f_hidg if the kref hits 0).

More than happy to revisit the first solution with Greg's blessing.

Quoted 42 lines · Lee Jones

Quoted 29 lines · Alan Stern

Quoted 28 lines · Lee Jones

Quoted 9 lines · Greg KH

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 92

Quoted 4 lines · Lee Jones

drivers/usb/gadget/function/f_hid.c +4 -0

+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}

Ick, sorry, no, that's not going to work and is not allowed at all. That's some major layering violations there, AND it can change after you get the value as well.

This cdev belongs solely to this driver. Hence the *.*.* and not *->*->*. What is preventing us from reading our own data? If we cannot do this directly, can I create an API to do it 'officially'?

I do, however, appreciate that a little locking wouldn't go amiss.

If this solution is not acceptable either, then we're left up the creak without a paddle. The rules you've communicated are not compatible with each other.

Rule 1: Only one item in a data structure can reference count.

Due to the embedded cdev struct, this rules out my first solution of giving f_hidg its own kref so that it can conduct its own life-time management.

Quoted 8 lines

Quoted 4 lines

A potential option to satisfy this rule would be to remove the cdev attribute and create its data dynamically instead. However, the staticness of cdev is used to obtain f_hidg (with container_of()) in the character device handling component, so it cannot be removed.

You have not understood this rule correctly. Only one item in a data structure can hold a reference count _for that structure_. But several items in a structure can hold reference counts for themselves.

Here was the review comment I was working to on this patch [0]:

Quoted 7 lines

"While at first glance, it seems that f_hidg is not reference
counted, it really is, with the embedded "struct cdev" a few lines above this.

That is the reference count that should control the lifecycle of this object, not another reference here in the "outer layer" structure."

It's worth noting that the review comment goes on to say:

"But, the cdev api is tricky and messy and not really set up to control
the lifecycle of objects it is embedded in."

This is a good indication that a separate reference counter really is needed (in fact it almost contradicts what was written above).

I don't think it's at all simple to fix this - I posted a series addressing the lifetime issues here a few years ago but didn't chase it up and there was no feedback:

	https://lore.kernel.org/linux-usb/[email protected]/

That includes a patch to remove the embedded struct cdev and manage its lifetime separately, which I think is needed as there are two different struct device objects here and we cannot tie their lifetimes together.

Quoted 9 lines · John Keeping

I don't think it's at all simple to fix this - I posted a series addressing the lifetime issues here a few years ago but didn't chase it up and there was no feedback:

	https://lore.kernel.org/linux-usb/[email protected]/

That includes a patch to remove the embedded struct cdev and manage its lifetime separately, which I think is needed as there are two different struct device objects here and we cannot tie their lifetimes together.

I still don't have a clear picture of what the real problem is. Lee's original patch description just said "external references are presently not tracked", with no details about what those external references are. Why not add just proper cdev_get() and cdev_put() calls to whatever code handles those external references, so that they _are_ tracked?

What are the two different struct device objects? Why do their lifetimes need to be tied together? If you do need to tie their lifetimes somehow, why not simply make one of them (the one which is logically allowed to be shorter-lived) hold a reference to the other?

Quoted 17 lines · Alan Stern

Quoted 9 lines · John Keeping

I don't think it's at all simple to fix this - I posted a series addressing the lifetime issues here a few years ago but didn't chase it up and there was no feedback:

	https://lore.kernel.org/linux-usb/[email protected]/

That includes a patch to remove the embedded struct cdev and manage its lifetime separately, which I think is needed as there are two different struct device objects here and we cannot tie their lifetimes together.

I still don't have a clear picture of what the real problem is. Lee's original patch description just said "external references are presently not tracked", with no details about what those external references are. Why not add just proper cdev_get() and cdev_put() calls to whatever code handles those external references, so that they _are_ tracked?

Quoted 4 lines

What are the two different struct device objects? Why do their lifetimes need to be tied together? If you do need to tie their lifetimes somehow, why not simply make one of them (the one which is logically allowed to be shorter-lived) hold a reference to the other?

The problem is that we have a struct cdev embedded in f_hidg but the lifetime of f_hidg is not tied to any kobject so we can't solve this in the right way by setting the parent kobject of the cdev.

While refcounting struct f_hidg is necessary, it's not sufficient because the only way to keep it alive long enough for the final kobject_put() on the embedded cdev is to tie the lifetime to a kobject of its own and there is no suitable object as this is not the model followed by gadget function instances.

Quoted 27 lines · John Keeping

Quoted 17 lines · Alan Stern

Quoted 9 lines · John Keeping

I don't think it's at all simple to fix this - I posted a series addressing the lifetime issues here a few years ago but didn't chase it up and there was no feedback:

	https://lore.kernel.org/linux-usb/[email protected]/

That includes a patch to remove the embedded struct cdev and manage its lifetime separately, which I think is needed as there are two different struct device objects here and we cannot tie their lifetimes together.

I still don't have a clear picture of what the real problem is. Lee's original patch description just said "external references are presently not tracked", with no details about what those external references are. Why not add just proper cdev_get() and cdev_put() calls to whatever code handles those external references, so that they _are_ tracked?

Quoted 4 lines

What are the two different struct device objects? Why do their lifetimes need to be tied together? If you do need to tie their lifetimes somehow, why not simply make one of them (the one which is logically allowed to be shorter-lived) hold a reference to the other?

The problem is that we have a struct cdev embedded in f_hidg but the lifetime of f_hidg is not tied to any kobject so we can't solve this in the right way by setting the parent kobject of the cdev.

Quoted 5 lines

While refcounting struct f_hidg is necessary, it's not sufficient because the only way to keep it alive long enough for the final kobject_put() on the embedded cdev is to tie the lifetime to a kobject of its own and there is no suitable object as this is not the model followed by gadget function instances.

I see. The solution is simple: Embed a struct device in struct f_hidg, and call cdev_device_add() to add the device and the cdev. This will automatically make the device the parent of the cdev, so the device's refcount won't go to 0 until the cdev's refcount does. Then you can tie the f_hidg's lifetime to the device's, so the device's release routine can safely deallocate the entire f_hidg structure.

The parent of the new struct device should be set to &gadget->dev. If you can't think of a better name for the device, you could simply append ":I" to the parent's name, where I is the interface number, or even append ":C.I" where C is the config number (like we do on the host side).

Quoted 27 lines · John Keeping

Quoted 17 lines · Alan Stern

Quoted 9 lines · John Keeping

I don't think it's at all simple to fix this - I posted a series addressing the lifetime issues here a few years ago but didn't chase it up and there was no feedback:

	https://lore.kernel.org/linux-usb/[email protected]/

That includes a patch to remove the embedded struct cdev and manage its lifetime separately, which I think is needed as there are two different struct device objects here and we cannot tie their lifetimes together.

I still don't have a clear picture of what the real problem is. Lee's original patch description just said "external references are presently not tracked", with no details about what those external references are. Why not add just proper cdev_get() and cdev_put() calls to whatever code handles those external references, so that they _are_ tracked?

Quoted 4 lines

What are the two different struct device objects? Why do their lifetimes need to be tied together? If you do need to tie their lifetimes somehow, why not simply make one of them (the one which is logically allowed to be shorter-lived) hold a reference to the other?

The problem is that we have a struct cdev embedded in f_hidg but the lifetime of f_hidg is not tied to any kobject so we can't solve this in the right way by setting the parent kobject of the cdev.

Quoted 5 lines

While refcounting struct f_hidg is necessary, it's not sufficient because the only way to keep it alive long enough for the final kobject_put() on the embedded cdev is to tie the lifetime to a kobject of its own and there is no suitable object as this is not the model followed by gadget function instances.

I see. The solution is simple: Embed a struct device in struct f_hidg, and call cdev_device_add() to add the device and the cdev. This will automatically make the device the parent of the cdev, so the device's refcount won't go to 0 until the cdev's refcount does. Then you can tie the f_hidg's lifetime to the device's, so the device's release routine can safely deallocate the entire f_hidg structure.

The parent of the new struct device should be set to &gadget->dev. If you can't think of a better name for the device, you could simply append ":I" to the parent's name, where I is the interface number, or even append ":C.I" where C is the config number (like we do on the host side).

Quoted 7 lines · Alan Stern

I see. The solution is simple: Embed a struct device in struct f_hidg, and call cdev_device_add() to add the device and the cdev. This will automatically make the device the parent of the cdev, so the device's refcount won't go to 0 until the cdev's refcount does. Then you can tie the f_hidg's lifetime to the device's, so the device's release routine can safely deallocate the entire f_hidg structure.

Quoted 5 lines

The parent of the new struct device should be set to &gadget->dev. If you can't think of a better name for the device, you could simply append ":I" to the parent's name, where I is the interface number, or even append ":C.I" where C is the config number (like we do on the host side).

There is no gadget->dev at the time struct f_hidg is allocated.

AFAICT the device is the UDC, which is only associated with the gadget when it is bound. The functions are allocated earlier than this and I can't see any device associated with struct usb_function_instance.

The patch below does fix the issue, but I'm wondering if there's a deeper problem here that can only be properly solved by adding some device/kobject hierarchy to the config side of things.

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Add a device to f_hidg so that the cdev can properly take a reference to this and the structure will be freed only when the child device is released.

[Also fix refcount leak on an error path.]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 35 ++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

drivers/usb/gadget/function/f_hid.c +9 -0

index ca0a7d9eaa34..7ae97e5c4d20 100644
@@ -71,6 +71,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	struct device			dev;
 	int				minor;
 	struct cdev			cdev;
 	struct usb_function		func;

@@ -84,6 +85,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 
+static void hidg_release(struct device *dev)
+{
+	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+
+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 

drivers/usb/gadget/function/f_hid.c +13 -2

 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);

@@ -1266,6 +1274,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 {
 	struct f_hidg *hidg;
 	struct f_hid_opts *opts;
+	int ret;
 
 	/* allocate and initialize one new instance */
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);

@@ -1277,17 +1286,27 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	++opts->refcnt;
 
+	device_initialize(&hidg->dev);
+	hidg->dev.release = hidg_release;
+	ret = dev_set_name(&hidg->dev, "hidg%d", hidg->minor);
+	if (ret) {
+		--opts->refcnt;
+		mutex_unlock(&opts->lock);
+		return ERR_PTR(ret);
+	}
+
 	hidg->minor = opts->minor;
 	hidg->bInterfaceSubClass = opts->subclass;
 	hidg->bInterfaceProtocol = opts->protocol;
 	hidg->report_length = opts->report_length;
 	hidg->report_desc_length = opts->report_desc_length;
 	if (opts->report_desc) {
-		hidg->report_desc = kmemdup(opts->report_desc,
+		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
 					    opts->report_desc_length,
 					    GFP_KERNEL);
 		if (!hidg->report_desc) {
-			kfree(hidg);
+			put_device(&hidg->dev);
+			--opts->refcnt;
 			mutex_unlock(&opts->lock);
 			return ERR_PTR(-ENOMEM);
 		}

drivers/usb/gadget/function/f_hid.c +1 -0

+
 	return &hidg->func;
 }
 

Signature · 1 line

2.38.1

Quoted 14 lines · John Keeping

Quoted 7 lines · Alan Stern

I see. The solution is simple: Embed a struct device in struct f_hidg, and call cdev_device_add() to add the device and the cdev. This will automatically make the device the parent of the cdev, so the device's refcount won't go to 0 until the cdev's refcount does. Then you can tie the f_hidg's lifetime to the device's, so the device's release routine can safely deallocate the entire f_hidg structure.

Quoted 5 lines

The parent of the new struct device should be set to &gadget->dev. If you can't think of a better name for the device, you could simply append ":I" to the parent's name, where I is the interface number, or even append ":C.I" where C is the config number (like we do on the host side).

Quoted 5 lines

There is no gadget->dev at the time struct f_hidg is allocated.

AFAICT the device is the UDC, which is only associated with the gadget when it is bound. The functions are allocated earlier than this and I can't see any device associated with struct usb_function_instance.

Ah, that's a shame. All right, so be it.

Quoted 3 lines

The patch below does fix the issue, but I'm wondering if there's a deeper problem here that can only be properly solved by adding some device/kobject hierarchy to the config side of things.

I don't believe there's any deeper problem. If someone wants to have an fhidg device as a child of the gadget, I think it could be added and removed in the ->set_alt() and ->disable() callbacks. Or maybe the ->bind() and ->unbind() callbacks (I've never had to work with configfs so I'm not clear on the details).

It turns out there's already a device being created here, just not associated with the structure. Your suggestions around cdev_device_add() made me spot what's going on with that so the actual fix is to pull its lifetime up to match struct f_hidg.

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Pull the existing device up in to struct f_hidg and make use of the cdev_device_{add,del}() helpers. This changes the lifetime of the device object to match struct f_hidg, but note that it is still added and deleted at the same time.

[Also fix refcount leak on an error path.]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

drivers/usb/gadget/function/f_hid.c +0 -0

index ca0a7d9eaa34..0b94668a3812 100644

drivers/usb/gadget/function/f_hid.c +2 -1

+	status = cdev_device_add(&hidg->cdev, &hidg->dev);
 	if (status)
 		goto fail_free_descs;
 

@@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	++opts->refcnt;
 
-	hidg->minor = opts->minor;
+	device_initialize(&hidg->dev);

Quoted 4 lines · John Keeping

It turns out there's already a device being created here, just not associated with the structure. Your suggestions around cdev_device_add() made me spot what's going on with that so the actual fix is to pull its lifetime up to match struct f_hidg.

It's not obvious from the patch what device was already being created, but never mind...

Quoted 34 lines

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Pull the existing device up in to struct f_hidg and make use of the cdev_device_{add,del}() helpers. This changes the lifetime of the device object to match struct f_hidg, but note that it is still added and deleted at the same time.

[Also fix refcount leak on an error path.]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

drivers/usb/gadget/function/f_hid.c +0 -0

index ca0a7d9eaa34..0b94668a3812 100644
@@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 

drivers/usb/gadget/function/f_hid.c static int hidg_bind(struct usb_configuration *c, struct usb_function *f) · line 1008

Quoted 5 lines

drivers/usb/gadget/function/f_hid.c +1 -2

 	/* create char device */
 	cdev_init(&hidg->cdev, &f_hidg_fops);
-	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
+	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);

This line isn't needed; cdev_device_add() does it for you because hidg->dev.devt has been set.

Quoted 27 lines · John Keeping

Quoted 14 lines · Alan Stern

Quoted 7 lines

I see. The solution is simple: Embed a struct device in struct f_hidg, and call cdev_device_add() to add the device and the cdev. This will automatically make the device the parent of the cdev, so the device's refcount won't go to 0 until the cdev's refcount does. Then you can tie the f_hidg's lifetime to the device's, so the device's release routine can safely deallocate the entire f_hidg structure.

Quoted 5 lines

The parent of the new struct device should be set to &gadget->dev. If you can't think of a better name for the device, you could simply append ":I" to the parent's name, where I is the interface number, or even append ":C.I" where C is the config number (like we do on the host side).

Quoted 5 lines

There is no gadget->dev at the time struct f_hidg is allocated.

AFAICT the device is the UDC, which is only associated with the gadget when it is bound. The functions are allocated earlier than this and I can't see any device associated with struct usb_function_instance.

Ah, that's a shame. All right, so be it.

Quoted 3 lines

The patch below does fix the issue, but I'm wondering if there's a deeper problem here that can only be properly solved by adding some device/kobject hierarchy to the config side of things.

Quoted 5 lines

I don't believe there's any deeper problem. If someone wants to have an fhidg device as a child of the gadget, I think it could be added and removed in the ->set_alt() and ->disable() callbacks. Or maybe the ->bind() and ->unbind() callbacks (I've never had to work with configfs so I'm not clear on the details).

It turns out there's already a device being created here, just not associated with the structure. Your suggestions around cdev_device_add() made me spot what's going on with that so the actual fix is to pull its lifetime up to match struct f_hidg.

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Pull the existing device up in to struct f_hidg and make use of the cdev_device_{add,del}() helpers. This changes the lifetime of the device object to match struct f_hidg, but note that it is still added and deleted at the same time.

[Also fix refcount leak on an error path.]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

drivers/usb/gadget/function/f_hid.c +1 -1

index ca0a7d9eaa34..0b94668a3812 100644
@@ -71,7 +71,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
-	int				minor;
+	struct device			dev;
 	struct cdev			cdev;
 	struct usb_function		func;
 

drivers/usb/gadget/function/f_hid.c +4 -2

+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 

@@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	struct usb_ep		*ep;
 	struct f_hidg		*hidg = func_to_hidg(f);
 	struct usb_string	*us;
-	struct device		*device;
 	int			status;
-	dev_t			dev;
 
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,

drivers/usb/gadget/function/f_hid.c +4 -14

+	status = cdev_device_add(&hidg->cdev, &hidg->dev);
 	if (status)
 		goto fail_free_descs;
 
-	device = device_create(hidg_class, NULL, dev, NULL,
-			       "%s%d", "hidg", hidg->minor);
-	if (IS_ERR(device)) {
-		status = PTR_ERR(device);
-		goto del;
-	}
-
 	return 0;
-del:
-	cdev_del(&hidg->cdev);
 fail_free_descs:
 	usb_free_all_descriptors(f);
 fail:

@@ -1244,9 +1241,7 @@ static void hidg_free(struct usb_function *f)
 
 	hidg = func_to_hidg(f);
 	opts = container_of(f->fi, struct f_hid_opts, func_inst);
-	kfree(hidg->report_desc);
-	kfree(hidg->set_report_buf);
-	kfree(hidg);
+	put_device(&hidg->dev);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);

@@ -1256,8 +1251,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
 
-	device_destroy(hidg_class, MKDEV(major, hidg->minor));
-	cdev_del(&hidg->cdev);
+	cdev_device_del(&hidg->cdev);
 
 	usb_free_all_descriptors(f);
 }

@@ -1266,6 +1260,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 {
 	struct f_hidg *hidg;
 	struct f_hid_opts *opts;
+	int ret;
 
 	/* allocate and initialize one new instance */
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);

@@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);

Quoted 14 lines · John Keeping

Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Quoted 4 lines

Pull the existing device up in to struct f_hidg and make use of the cdev_device_{add,del}() helpers. This changes the lifetime of the device object to match struct f_hidg, but note that it is still added and deleted at the same time.

This is much better, thanks for re-spinning.

Quoted 24 lines

[Also fix refcount leak on an error path.]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

drivers/usb/gadget/function/f_hid.c +1 -1

index ca0a7d9eaa34..0b94668a3812 100644
@@ -71,7 +71,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
-	int				minor;
+	struct device			dev;
 	struct cdev			cdev;
 	struct usb_function		func;
 

@@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 

drivers/usb/gadget/function/f_hid.c static inline struct f_hidg *func_to_hidg(struct usb_function *f) · line 89

Quoted 3 lines

drivers/usb/gadget/function/f_hid.c +3 -0

+static void hidg_release(struct device *dev)
+{
+	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);

Could we store/fetch this with dev_{set,get}_drvdata(), and make hidg->dev a pointer reducing the size of the struct f_hidg.

Quoted 18 lines

drivers/usb/gadget/function/f_hid.c +4 -2

+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 

@@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	struct usb_ep		*ep;
 	struct f_hidg		*hidg = func_to_hidg(f);
 	struct usb_string	*us;
-	struct device		*device;
 	int			status;
-	dev_t			dev;
 
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,

drivers/usb/gadget/function/f_hid.c static int hidg_bind(struct usb_configuration *c, struct usb_function *f) · line 1008

Quoted 6 lines

drivers/usb/gadget/function/f_hid.c +1 -2

@@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 
 	/* create char device */
 	cdev_init(&hidg->cdev, &f_hidg_fops);
-	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
+	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);

cdev_device_add() should take care of this, so long as:

    if (dev->devt)
        dev_set_parent(cdev, &dev->kobj);

Quoted 50 lines

drivers/usb/gadget/function/f_hid.c +4 -14

+	status = cdev_device_add(&hidg->cdev, &hidg->dev);
 	if (status)
 		goto fail_free_descs;
 
-	device = device_create(hidg_class, NULL, dev, NULL,
-			       "%s%d", "hidg", hidg->minor);
-	if (IS_ERR(device)) {
-		status = PTR_ERR(device);
-		goto del;
-	}
-
 	return 0;
-del:
-	cdev_del(&hidg->cdev);
 fail_free_descs:
 	usb_free_all_descriptors(f);
 fail:

@@ -1244,9 +1241,7 @@ static void hidg_free(struct usb_function *f)
 
 	hidg = func_to_hidg(f);
 	opts = container_of(f->fi, struct f_hid_opts, func_inst);
-	kfree(hidg->report_desc);
-	kfree(hidg->set_report_buf);
-	kfree(hidg);
+	put_device(&hidg->dev);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);

@@ -1256,8 +1251,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
 
-	device_destroy(hidg_class, MKDEV(major, hidg->minor));
-	cdev_del(&hidg->cdev);
+	cdev_device_del(&hidg->cdev);
 
 	usb_free_all_descriptors(f);
 }

@@ -1266,6 +1260,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 {
 	struct f_hidg *hidg;
 	struct f_hid_opts *opts;
+	int ret;
 
 	/* allocate and initialize one new instance */
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);

@@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	++opts->refcnt;
 

drivers/usb/gadget/function/f_hid.c static struct usb_function *hidg_alloc(struct usb_function_instance *fi) · line 1281

Quoted 8 lines

drivers/usb/gadget/function/f_hid.c +7 -1

-	hidg->minor = opts->minor;
+	device_initialize(&hidg->dev);
+	hidg->dev.release = hidg_release;
+	hidg->dev.class = hidg_class;
+	hidg->dev.devt = MKDEV(major, opts->minor);
+	ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor);
+	if (ret) {
+		--opts->refcnt;

Since we're holding the opts lock at this point, is there anything preventing us from incrementing the refcnt at the end, just before giving up the lock, thus saving 2 decrements in the error paths?

drivers/usb/gadget/function/f_hid.c static struct usb_function *hidg_alloc(struct usb_function_instance *fi) · line 1291

Quoted 11 lines

drivers/usb/gadget/function/f_hid.c +5 -1

+		mutex_unlock(&opts->lock);
+		return ERR_PTR(ret);
+	}
+
 	hidg->bInterfaceSubClass = opts->subclass;
 	hidg->bInterfaceProtocol = opts->protocol;
 	hidg->report_length = opts->report_length;
 	hidg->report_desc_length = opts->report_desc_length;
 	if (opts->report_desc) {
-		hidg->report_desc = kmemdup(opts->report_desc,
+		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,

Nice.

drivers/usb/gadget/function/f_hid.c static struct usb_function *hidg_alloc(struct usb_function_instance *fi) · line 1299

Quoted 9 lines

drivers/usb/gadget/function/f_hid.c +2 -1

 					    opts->report_desc_length,
 					    GFP_KERNEL);
 		if (!hidg->report_desc) {
-			kfree(hidg);
+			put_device(&hidg->dev);
+			--opts->refcnt;
 			mutex_unlock(&opts->lock);
 			return ERR_PTR(-ENOMEM);
 		}

Thanks for doing this John, your work is appreciated.