From: Subbaraman Narayanamurthy <quic_subbaram@quicinc.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>,
Zhang Rui <rui.zhang@intel.com>, Amit Kucheria <amitk@kernel.org>
Cc: <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
David Collins <quic_collinsd@quicinc.com>,
Manaf Meethalavalappu Pallikunhi <manafm@codeaurora.org>,
Ram Chandrasekar <rkumbako@codeaurora.org>,
<stable@vger.kernel.org>
Subject: Re: [PATCH v2] thermal: Fix a NULL pointer dereference
Date: Tue, 5 Oct 2021 15:09:39 -0700 [thread overview]
Message-ID: <f869ea55-f071-f971-282e-31878a0f0b68@quicinc.com> (raw)
In-Reply-To: <7930989e-baf1-04f4-59ad-d65122149b9b@quicinc.com>
On 9/17/21 1:06 PM, Subbaraman Narayanamurthy wrote:
> On 9/17/21 2:31 AM, Daniel Lezcano wrote:
>> On 07/09/2021 21:01, Subbaraman Narayanamurthy wrote:
>>> of_parse_thermal_zones() parses the thermal-zones node and registers a
>>> thermal_zone device for each subnode. However, if a thermal zone is
>>> consuming a thermal sensor and that thermal sensor device hasn't probed
>>> yet, an attempt to set trip_point_*_temp for that thermal zone device
>>> can cause a NULL pointer dereference. Fix it.
>>>
>>> console:/sys/class/thermal/thermal_zone87 # echo 120000 > trip_point_0_temp
>>> ...
>>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
>>> ...
>>> Call trace:
>>> of_thermal_set_trip_temp+0x40/0xc4
>>> trip_point_temp_store+0xc0/0x1dc
>>> dev_attr_store+0x38/0x88
>>> sysfs_kf_write+0x64/0xc0
>>> kernfs_fop_write_iter+0x108/0x1d0
>>> vfs_write+0x2f4/0x368
>>> ksys_write+0x7c/0xec
>>> __arm64_sys_write+0x20/0x30
>>> el0_svc_common.llvm.7279915941325364641+0xbc/0x1bc
>>> do_el0_svc+0x28/0xa0
>>> el0_svc+0x14/0x24
>>> el0_sync_handler+0x88/0xec
>>> el0_sync+0x1c0/0x200
>>>
>>> While at it, fix the possible NULL pointer dereference in other
>>> functions as well: of_thermal_get_temp(), of_thermal_set_emul_temp(),
>>> of_thermal_get_trend().
>>>
>>> Cc: stable@vger.kernel.org
>>> Suggested-by: David Collins <quic_collinsd@quicinc.com>
>>> Signed-off-by: Subbaraman Narayanamurthy <quic_subbaram@quicinc.com>
>>> ---
>>> Changes for v2:
>>> - Added checks in of_thermal_get_temp(), of_thermal_set_emul_temp(), of_thermal_get_trend().
>>>
>>> drivers/thermal/thermal_of.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>>> index 6379f26..9233f7e 100644
>>> --- a/drivers/thermal/thermal_of.c
>>> +++ b/drivers/thermal/thermal_of.c
>>> @@ -89,7 +89,7 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>> {
>>> struct __thermal_zone *data = tz->devdata;
>>>
>>> - if (!data->ops->get_temp)
>>> + if (!data->ops || !data->ops->get_temp)
>> comment (1)
>>
>> AFAICT, if data->ops != NULL then data->ops->get_temp is also != NULL
>> because of the code allocating/freeing the ops structure.
>>
>> The tests can be replaced by (!data->ops), no ?
> Thanks Daniel for reviewing the patch.
>
> I agree that even if a sensor module is unregistered, that would call
> "thermal_zone_of_sensor_unregister" which would eventually set NULL on
> get_temp() and get_trend() and "tzd->ops" as well.
>
> However, of_thermal_get_temp() is trying to call "data->ops->get_temp"
> which comes from a sensor driver when it registers. There is no
> guarantee that it would be non-NULL right?
>
> Thinking of which, I think having both checks would be valid.
Hi Daniel,
Do you still have any concerns with this change?
Thanks,
Subbaraman
>>> return -EINVAL;
>>>
>>> return data->ops->get_temp(data->sensor_data, temp);
>>> @@ -186,6 +186,9 @@ static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
>>> {
>>> struct __thermal_zone *data = tz->devdata;
>>>
>>> + if (!data->ops || !data->ops->set_emul_temp)
>>> + return -EINVAL;
>>> +
>> comment (2)
>>
>> The test looks pointless here (I mean both of them).
>>
>> If of_thermal_set_emul_temp() is called it is because the callback was
>> set in thermal_zone_of_add_sensor().
>>
>> This one does:
>>
>> tz->ops = ops;
>>
>> and
>> if (ops->set_emul_temp)
>> tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>>
>> If I'm not wrong if we are called, then data->ops &&
>> data->ops->set_emul_temp is always true, right ?
>>
> I've not exercised this condition yet. However, the original problem
> we've observed was when thermal HAL was trying to set trip thresholds
> on a thermal zone device for which the sensor device is not probed yet.
> This had happened randomly because of vendor modules taking time to be
> loaded and probed. I don't know if there would be any userspace entity
> that can try to set emulated temperature for a thermal zone even before
> a sensor device is probed.
>
> Without a sensor driver probed, "tz->ops" would not have a valid pointer
> right? So, I think checking for "data->ops" should be good.
>
> Another possibility is, a sensor might not have "set_emul_temp" callback.
> So checking for "ops->set_emul_temp" should be still valid.
>
>>> return data->ops->set_emul_temp(data->sensor_data, temp);
>>> }
>>>
>>> @@ -194,7 +197,7 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>>> {
>>> struct __thermal_zone *data = tz->devdata;
>>>
>>> - if (!data->ops->get_trend)
>>> + if (!data->ops || !data->ops->get_trend)
>>> return -EINVAL;
>> Same comment as (1)
> of_thermal_get_trend() is trying to call "data->ops->get_trend" which
> comes from a sensor driver when it registers. From what I can see,
> there are lot of drivers which don't pass "get_trend" in their ops.
> So there is no guarantee that it would be non-NULL right?
>
> Thinking of which, I think having both checks would be valid.
>
>>> return data->ops->get_trend(data->sensor_data, trip, trend);
>>> @@ -301,7 +304,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>> if (trip >= data->ntrips || trip < 0)
>>> return -EDOM;
>>>
>>> - if (data->ops->set_trip_temp) {
>>> + if (data->ops && data->ops->set_trip_temp) {
>> Same comment as (2)
> Without a sensor driver probed, "tz->ops" would not have a valid pointer right?
> So, I think checking for "data->ops" should be good. Another possibility is, a
> sensor device might not have "set_trip_temp" callback but just "set_trips".
>
> So checking for "data->ops->set_trip_temp" might be still valid.
>
>>> int ret;
>>>
>>> ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
>>>
next prev parent reply other threads:[~2021-10-05 22:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-07 19:01 [PATCH v2] thermal: Fix a NULL pointer dereference Subbaraman Narayanamurthy
2021-09-17 9:31 ` Daniel Lezcano
2021-09-17 20:06 ` Subbaraman Narayanamurthy
2021-10-05 22:09 ` Subbaraman Narayanamurthy [this message]
2021-10-06 9:39 ` Daniel Lezcano
2021-10-06 11:08 ` Daniel Lezcano
2021-10-08 19:50 ` Subbaraman Narayanamurthy
2021-10-19 1:21 ` Subbaraman Narayanamurthy
2021-10-19 7:35 ` Daniel Lezcano
2021-10-21 18:29 ` Subbaraman Narayanamurthy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f869ea55-f071-f971-282e-31878a0f0b68@quicinc.com \
--to=quic_subbaram@quicinc.com \
--cc=amitk@kernel.org \
--cc=daniel.lezcano@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=manafm@codeaurora.org \
--cc=quic_collinsd@quicinc.com \
--cc=rkumbako@codeaurora.org \
--cc=rui.zhang@intel.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).