lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 11 Dec 2020 15:12:53 +0100
From:   Hannes Reinecke <hare@...e.de>
To:     Enzo Matsumiya <ematsumiya@...e.de>, linux-nvme@...ts.infradead.org
Cc:     Sagi Grimberg <sagi@...mberg.me>, linux-kernel@...r.kernel.org,
        Jens Axboe <axboe@...com>, Keith Busch <kbusch@...nel.org>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] nvme: hwmon: fix crash on device teardown

On 12/9/20 10:32 PM, Enzo Matsumiya wrote:
> Fix a possible NULL pointer dereference when trying to read
> hwmon sysfs entries associated to NVMe-oF devices that were
> hot-removed or disconnected.
> 
> Unregister the NVMe hwmon device upon controller teardown
> (nvme_stop_ctrl()).
> 
> Signed-off-by: Enzo Matsumiya <ematsumiya@...e.de>
> ---
>   drivers/nvme/host/core.c  | 1 +
>   drivers/nvme/host/hwmon.c | 8 ++++++++
>   drivers/nvme/host/nvme.h  | 2 ++
>   3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9a270e49df17..becc80a0c3b8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4344,6 +4344,7 @@ EXPORT_SYMBOL_GPL(nvme_complete_async_event);
>   
>   void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>   {
> +	nvme_hwmon_exit(ctrl);
>   	nvme_mpath_stop(ctrl);
>   	nvme_stop_keep_alive(ctrl);
>   	flush_work(&ctrl->async_event_work);
> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 552dbc04567b..7f62cca4c577 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
> @@ -71,6 +71,9 @@ static int nvme_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>   	int temp;
>   	int err;
>   
> +	if (data->ctrl->state != NVME_CTRL_LIVE)
> +		return -EAGAIN;
> +
>   	/*
>   	 * First handle attributes which don't require us to read
>   	 * the smart log.
> @@ -253,3 +256,8 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>   
>   	return 0;
>   }
> +
> +void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
> +{
> +	hwmon_device_unregister(ctrl->dev);
> +}
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 567f7ad18a91..621e9b1575f6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -807,11 +807,13 @@ static inline struct nvme_ns *nvme_get_ns_from_dev(struct device *dev)
>   
>   #ifdef CONFIG_NVME_HWMON
>   int nvme_hwmon_init(struct nvme_ctrl *ctrl);
> +void nvme_hwmon_exit(struct nvme_ctrl *ctrl);
>   #else
>   static inline int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>   {
>   	return 0;
>   }
> +static inline void nvme_hwmon_exit(struct nvme_ctrl *ctrl) { }
>   #endif
>   
>   u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> 
Something's fishy here.
The hwmon attributes should have been created only once for the lifetime 
of the controller (creation is protected by '!initialized').
And the hwmon lifetime should've been controlled by the lifetime of the 
controller (which to my knowledge was the idea behind the devm_* thingies).
So why do we have to deallocate the hwmon attributes?
And why on reset? And who's re-creating them after reset, seeing that 
'initialized' should be true?
Hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@...e.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Powered by blists - more mailing lists