[1/2] devcoredump: Remove devcoredump device if failing device is gone

  Make dev_coredumpm a real device managed helper, that not only
frees the device after a scheduled delay (DEVCD_TIMEOUT), but
also when the failing/crashed device is gone.

The module remove for the drivers using devcoredump are currently
broken if attempted between the crash and the DEVCD_TIMEOUT, since
the symbolic sysfs link won't be deleted.

On top of that, for PCI devices, the unbind of the device will
call the pci .remove void function, that cannot fail. At that
time, our device is pretty much gone, but the read and free
functions are alive trough the devcoredump device and they
can get some NULL dereferences or use after free.

So, if the failing-device is gone let's also request for the
devcoredump-device removal using the same mod_delayed_work
as when writing anything through data. The flush cannot be
used since it is synchronous and the devcd would be surely
gone right before the mutex_unlock on the next line.

Cc: Jose Souza <jose.souza@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/base/devcoredump.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
  

On Fri, 2024-01-26 at 10:11 -0500, Rodrigo Vivi wrote:
> Make dev_coredumpm a real device managed helper, that not only
> frees the device after a scheduled delay (DEVCD_TIMEOUT), but
> also when the failing/crashed device is gone.
> 
> The module remove for the drivers using devcoredump are currently
> broken if attempted between the crash and the DEVCD_TIMEOUT, since
> the symbolic sysfs link won't be deleted.
> 
> On top of that, for PCI devices, the unbind of the device will
> call the pci .remove void function, that cannot fail. At that
> time, our device is pretty much gone, but the read and free
> functions are alive trough the devcoredump device and they
> can get some NULL dereferences or use after free.
> 
> So, if the failing-device is gone let's also request for the
> devcoredump-device removal using the same mod_delayed_work
> as when writing anything through data. The flush cannot be
> used since it is synchronous and the devcd would be surely
> gone right before the mutex_unlock on the next line.
> 
> 
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Cc: Jose Souza <jose.souza@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/base/devcoredump.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index 7e2d1f0d903a..678ecc2fa242 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -304,6 +304,19 @@ static ssize_t devcd_read_from_sgtable(char *buffer, loff_t offset,
>  				  offset);
>  }
>  
> +static void devcd_remove(void *data)
> +{
> +	struct devcd_entry *devcd = data;
> +
> +	mutex_lock(&devcd->mutex);
> +	if (!devcd->delete_work) {
> +		devcd->delete_work = true;
> +		/* XXX: Cannot flush otherwise the mutex below will hit a UAF */
> +		mod_delayed_work(system_wq, &devcd->del_wk, 0);
> +	}
> +	mutex_unlock(&devcd->mutex);
> +}
> +
>  /**
>   * dev_coredumpm - create device coredump with read/free methods
>   * @dev: the struct device for the crashed device
> @@ -381,6 +394,8 @@ void dev_coredumpm(struct device *dev, struct module *owner,
>  	kobject_uevent(&devcd->devcd_dev.kobj, KOBJ_ADD);
>  	INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
>  	schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
> +	if (devm_add_action(dev, devcd_remove, devcd))
> +		dev_warn(dev, "devcoredump managed auto-removal registration failed\n");
>  	mutex_unlock(&devcd->mutex);
>  	return;
>   put_device:
  

On Fri, 2024-01-26 at 10:11 -0500, Rodrigo Vivi wrote:
> Make dev_coredumpm a real device managed helper, that not only
> frees the device after a scheduled delay (DEVCD_TIMEOUT), but
> also when the failing/crashed device is gone.
> 
> The module remove for the drivers using devcoredump are currently
> broken if attempted between the crash and the DEVCD_TIMEOUT, since
> the symbolic sysfs link won't be deleted.

Hmm, is it a problem to remove a whole dev when it still has some link
here? Maybe we could just make the link be managed/auto-removed?

Probably regardless of that you should change the comment in
devcd_dev_release() since it's no longer a concern?

> On top of that, for PCI devices, the unbind of the device will
> call the pci .remove void function, that cannot fail. At that
> time, our device is pretty much gone, but the read and free
> functions are alive trough the devcoredump device and they
                      ^ through, I guess

> can get some NULL dereferences or use after free.

Not sure I understand this part, how's this related to PCI's .remove?

> So, if the failing-device is gone let's also request for the
> devcoredump-device removal using the same mod_delayed_work
> as when writing anything through data. The flush cannot be
> used since it is synchronous and the devcd would be surely
> gone right before the mutex_unlock on the next line.

Can we just decouple it instead and remove the symlink? Which is kind of
what the comment in devcd_dev_release() says but at the time I wasn't
aware of all the devm mechanics etc.

I'm thinking this might be annoying in certain recovery cases, e.g.
iwlwifi uses this but may sometimes unbind/rebind itself to recover from
certain errors, and that'd make the FW dumps disappear.

johannes
  

On Mon, Jan 29, 2024 at 06:48:12PM +0100, Johannes Berg wrote:
> On Fri, 2024-01-26 at 10:11 -0500, Rodrigo Vivi wrote:
> > Make dev_coredumpm a real device managed helper, that not only
> > frees the device after a scheduled delay (DEVCD_TIMEOUT), but
> > also when the failing/crashed device is gone.
> > 
> > The module remove for the drivers using devcoredump are currently
> > broken if attempted between the crash and the DEVCD_TIMEOUT, since
> > the symbolic sysfs link won't be deleted.
> 
> Hmm, is it a problem to remove a whole dev when it still has some link
> here? 

Well, the big problem is that with link there, the base sysfs dir
is not deleted/removed. So, the next reload fails to recreate beucase
file exists.

> Maybe we could just make the link be managed/auto-removed?

this would help this angle indeed.

> 
> Probably regardless of that you should change the comment in
> devcd_dev_release() since it's no longer a concern?

indeed, I don't believe that that is a concern because this
is exactly the only place deleting the link and it can't race
with itself.

> 
> > On top of that, for PCI devices, the unbind of the device will
> > call the pci .remove void function, that cannot fail. At that
> > time, our device is pretty much gone, but the read and free
> > functions are alive trough the devcoredump device and they
>                       ^ through, I guess
> 
> > can get some NULL dereferences or use after free.
> 
> Not sure I understand this part, how's this related to PCI's .remove?

Well, this is my secondary concern that the idea of the link_auto_removal
doesn't cover.

If the failing_device is gone, the 'data cookie' it used to register with
dev_coredumpm(... void *data,...), is also likely gone on a clean removal.

And to be honest, we shouldn't even count that the registered *read()
function pointer is valid anymore.

I'm sorry for not being clear on this point. The other one was the
immediate one blocking our CI so I ended up writing up the commit
message with that in mind and without thinking about alternatives
like only removing the link.

> 
> > So, if the failing-device is gone let's also request for the
> > devcoredump-device removal using the same mod_delayed_work
> > as when writing anything through data. The flush cannot be
> > used since it is synchronous and the devcd would be surely
> > gone right before the mutex_unlock on the next line.
> 
> Can we just decouple it instead and remove the symlink? Which is kind of
> what the comment in devcd_dev_release() says but at the time I wasn't
> aware of all the devm mechanics etc.

Well, we could indeed. And that would unblock our CI, but I'm afraid
it wouldn't protect the final user from bad memory access on a direct
$ cat /sys/class/devcoredump/devcd<n>/data

Shouldn't we consider this critical itself to justify this entirely
removal?

> 
> I'm thinking this might be annoying in certain recovery cases, e.g.
> iwlwifi uses this but may sometimes unbind/rebind itself to recover from
> certain errors, and that'd make the FW dumps disappear.

I see... but it looks like dev_coredumpsg have a different handle of
the data cookie and read functions with the read direct from the
sgtable and might not face this bad memory access, since it allocates
the sg_dump_data which is only deleted/freed by the devcoredump removal...

But I'm concerned with the direct usage of the drivers using
dev_coredumpm() directly.

Should I then move dev_coredumpm to  _dev_coredumpm()
and then create a new dev_coredumpm that calls for
_dev_coredump and this devm_add_action(dev, devcd_remove, devcd)

?

And also an improved commit message to show the bad memory access issue?

Thank you so much for the feedback,
Rodrigo.

> 
> johannes
  

On Mon, 2024-01-29 at 16:29 -0500, Rodrigo Vivi wrote:
> > 
> > > On top of that, for PCI devices, the unbind of the device will
> > > call the pci .remove void function, that cannot fail. At that
> > > time, our device is pretty much gone, but the read and free
> > > functions are alive trough the devcoredump device and they
> >                       ^ through, I guess
> > 
> > > can get some NULL dereferences or use after free.
> > 
> > Not sure I understand this part, how's this related to PCI's .remove?
> 
> Well, this is my secondary concern that the idea of the link_auto_removal
> doesn't cover.
> 
> If the failing_device is gone, the 'data cookie' it used to register with
> dev_coredumpm(... void *data,...), is also likely gone on a clean removal.

That's on the user. You'll always be able to shoot yourself in the foot.

> And to be honest, we shouldn't even count that the registered *read()
> function pointer is valid anymore.

That's not true: the module cannot be removed, there's a reference to it
if you're using dev_coredumpm() correctly (which is to say: pass
THIS_MODULE to the struct module *owner argument).

> Well, we could indeed. And that would unblock our CI, but I'm afraid
> it wouldn't protect the final user from bad memory access on a direct
> $ cat /sys/class/devcoredump/devcd<n>/data
> 
> Shouldn't we consider this critical itself to justify this entirely
> removal?

No? IMHO that's totally on the user. If you absolutely cannot make a
standalone dump 'data' pointer (why not?! you can always stick the
actual data into a vmalloc chunk and use dev_coredumpv()?) then maybe we
can offer ways of removing it when you need to? But I'd rather not, it
feels weird to have a need for it.

johannes
  

> > If the failing_device is gone, the 'data cookie' it used to register with
> > dev_coredumpm(... void *data,...), is also likely gone on a clean removal.
> 
> That's on the user. You'll always be able to shoot yourself in the foot.
> 
> > And to be honest, we shouldn't even count that the registered *read()
> > function pointer is valid anymore.
> 
> That's not true: the module cannot be removed, there's a reference to it
> if you're using dev_coredumpm() correctly (which is to say: pass
> THIS_MODULE to the struct module *owner argument).
> 
> > Well, we could indeed. And that would unblock our CI, but I'm afraid
> > it wouldn't protect the final user from bad memory access on a direct
> > $ cat /sys/class/devcoredump/devcd<n>/data
> > 
> > Shouldn't we consider this critical itself to justify this entirely
> > removal?
> 
> No? IMHO that's totally on the user. If you absolutely cannot make a
> standalone dump 'data' pointer (why not?! you can always stick the
> actual data into a vmalloc chunk and use dev_coredumpv()?)

hmm... fair enough. We would be okay here indeed since devcoredump always
free the data.

> then maybe we
> can offer ways of removing it when you need to?

well, to be honest my first local version was like that, offering
a dev_coredump_removal() that driver could request the removal
before going away.

> But I'd rather not, it
> feels weird to have a need for it.

We could change or CI and instruct our devs to always write
something to 'data' to ensure that devcoredump is deleted
before we can reload our module. Maybe that's the right
approach indeed, although I would really prefer to have
a direct way.


> 
> johannes
  

On Tue, 2024-01-30 at 10:16 -0500, Rodrigo Vivi wrote:
> > 
> > But I'd rather not, it
> > feels weird to have a need for it.
> 
> We could change or CI and instruct our devs to always write
> something to 'data' to ensure that devcoredump is deleted
> before we can reload our module. Maybe that's the right
> approach indeed, although I would really prefer to have
> a direct way.

That's not really what I meant :-) I think we can agree that it's wrong
for the kernel to be _able_ to run into some kind of use-after-free if
userspace isn't doing the right thing here!

What I meant though is: it's weird for 'data' to actually depend on the
struct device being still around, no? Whatever you want 'data' to be,
couldn't you arrange it so that it's valid as long as the module isn't
removed, so that the 'data' pointer literally encapsulates the needed
data, doesn't depend on anything else, and the method you pass is more
like a 'format' method.

johannes
  

On Tue, Jan 30, 2024 at 04:19:18PM +0100, Johannes Berg wrote:
> On Tue, 2024-01-30 at 10:16 -0500, Rodrigo Vivi wrote:
> > > 
> > > But I'd rather not, it
> > > feels weird to have a need for it.
> > 
> > We could change or CI and instruct our devs to always write
> > something to 'data' to ensure that devcoredump is deleted
> > before we can reload our module. Maybe that's the right
> > approach indeed, although I would really prefer to have
> > a direct way.
> 
> That's not really what I meant :-) I think we can agree that it's wrong
> for the kernel to be _able_ to run into some kind of use-after-free if
> userspace isn't doing the right thing here!
> 
> What I meant though is: it's weird for 'data' to actually depend on the
> struct device being still around, no? Whatever you want 'data' to be,
> couldn't you arrange it so that it's valid as long as the module isn't
> removed, so that the 'data' pointer literally encapsulates the needed
> data, doesn't depend on anything else, and the method you pass is more
> like a 'format' method.

I'm sorry for not being clear here. I totally agree with you.

I will make changes to our driver to make the 'data' a standalone memory
that devcoredump will free. this ensures no uaf and no null deref.
data could be read even after unbinding the driver.

What I meant to userspace 'writing to 'data'' was to ensure that
on our CI we run something like

if /sys/.../device/devcd<n> exists, then
echo 1 > /sys/.../device/devcd<n>/data
before attempting the rmmod <driver>

our rmmod cannot get stuck or our CI is blocked, but then ensuring
the devcd is gone with module_put happening is the only current way
of not blocking the rmmod.

> 
> johannes
  

On Tue, 2024-01-30 at 10:49 -0500, Rodrigo Vivi wrote:
> 
> I will make changes to our driver to make the 'data' a standalone memory
> that devcoredump will free. this ensures no uaf and no null deref.
> data could be read even after unbinding the driver.
> 
> What I meant to userspace 'writing to 'data'' was to ensure that
> on our CI we run something like
> 
> if /sys/.../device/devcd<n> exists, then
> echo 1 > /sys/.../device/devcd<n>/data
> before attempting the rmmod <driver>
> 
> our rmmod cannot get stuck or our CI is blocked, but then ensuring
> the devcd is gone with module_put happening is the only current way
> of not blocking the rmmod.
> 

Ah, you were just concerned about the module removal, sure, that makes
sense.

Though depending on how you make that data pointer: if you just use
*sg() or *v() then you don't have this problem in the first place. OTOH,
it's probably good to have a udev rule to automatically capture the data
in CI anyway (and fail the test if it happened?)

johannes
  

On 1/29/2024 11:18 PM, Johannes Berg wrote:
> On Fri, 2024-01-26 at 10:11 -0500, Rodrigo Vivi wrote:
>> Make dev_coredumpm a real device managed helper, that not only
>> frees the device after a scheduled delay (DEVCD_TIMEOUT), but
>> also when the failing/crashed device is gone.
>>
>> The module remove for the drivers using devcoredump are currently
>> broken if attempted between the crash and the DEVCD_TIMEOUT, since
>> the symbolic sysfs link won't be deleted.
> 
> Hmm, is it a problem to remove a whole dev when it still has some link
> here? Maybe we could just make the link be managed/auto-removed?
> 
> Probably regardless of that you should change the comment in
> devcd_dev_release() since it's no longer a concern?
> 
>> On top of that, for PCI devices, the unbind of the device will
>> call the pci .remove void function, that cannot fail. At that
>> time, our device is pretty much gone, but the read and free
>> functions are alive trough the devcoredump device and they
>                        ^ through, I guess
> 
>> can get some NULL dereferences or use after free.
> 
> Not sure I understand this part, how's this related to PCI's .remove?
> 
>> So, if the failing-device is gone let's also request for the
>> devcoredump-device removal using the same mod_delayed_work
>> as when writing anything through data. The flush cannot be
>> used since it is synchronous and the devcd would be surely
>> gone right before the mutex_unlock on the next line.
> 
> Can we just decouple it instead and remove the symlink? Which is kind of
> what the comment in devcd_dev_release() says but at the time I wasn't
> aware of all the devm mechanics etc.

Are we going to do this ?

-Mukesh

> 
> I'm thinking this might be annoying in certain recovery cases, e.g.
> iwlwifi uses this but may sometimes unbind/rebind itself to recover from
> certain errors, and that'd make the FW dumps disappear.
> 
> johannes
>