| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 03 Feb 2013 13:17:16 +0200
From: Andrey Gelman <andreyg@...pulab.co.il>
To: Anton Vorontsov <anton@...msg.org>
CC: dwmw2@...radead.org, linux-kernel@...r.kernel.org
Subject: Re: A patch to test_power module
On 02/03/2013 05:39 AM, Anton Vorontsov wrote:
> On Sun, Jan 27, 2013 at 10:45:43PM +0200, Andrey Gelman wrote:
>> Hello !
>> This patch fixes 2 issues in test_power module:
>> 1. the mapping of ac_online parameter to its string value "on / off"
>> was swapped.
>> 2. more important ...
>> You could not insmod test_power with module parameters, without
>> causing kernel crash.
>> The reason is that in test_power, setting module parameter value has
>> side effect - power_supply_changed event.
>> As the module parameters are processed prior to calling module_init,
>> power_supply_changed event is generated on behalf module that is not
>> yet initialized, causing some NULL pointer dereference.
>>
>> My solution is to test whether the module has been initialized,
>> prior to calling power_supply_changed.
>
> Hi!
>
> The patch looks good, but it is missing Signed-off-by tag. Plus, you
> should also Cc: linux-kernel@...r.kernel.org on submissions (I've added it
> now).
>
> Plus, please don't use html in emails. :)
>
> Thanks a lot!
>
> Anton
>
>> From ccd5b3e916477a3c2a51bbf10a3be8e01f3c85e4 Mon Sep 17 00:00:00 2001
>> From: Andrey Gelman <andreyg@...pulab.co.il>
>> Date: Sun, 27 Jan 2013 22:16:33 +0200
>> Subject: [PATCH] test_power: fix a bug in setting module parameter values
>>
>> When the kernel loads a module, module params are processed
>> prior to calling module_init. As a result, setting module param
>> value should not have side effects, at least as long as the
>> module has not been initialized.
>> ---
>> drivers/power/test_power.c | 31 +++++++++++++++++++++----------
>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
>> index b99a452..753a74f 100644
>> --- a/drivers/power/test_power.c
>> +++ b/drivers/power/test_power.c
>> @@ -30,6 +30,8 @@ static int battery_technology = POWER_SUPPLY_TECHNOLOGY_LION;
>> static int battery_capacity = 50;
>> static int battery_voltage = 3300;
>>
>> +static bool module_initialized = false;
>> +
>> static int test_power_get_ac_property(struct power_supply *psy,
>> enum power_supply_property psp,
>> union power_supply_propval *val)
>> @@ -185,6 +187,7 @@ static int __init test_power_init(void)
>> }
>> }
>>
>> + module_initialized = true;
>> return 0;
>> failed:
>> while (--i >= 0)
>> @@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
>>
>> for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
>> power_supply_unregister(&test_power_supplies[i]);
>> +
>> + module_initialized = false;
>> }
>> module_exit(test_power_exit);
>>
>> @@ -221,8 +226,8 @@ struct battery_property_map {
>> };
>>
>> static struct battery_property_map map_ac_online[] = {
>> - { 0, "on" },
>> - { 1, "off" },
>> + { 0, "off" },
>> + { 1, "on" },
>> { -1, NULL },
>> };
>>
>> @@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
>> return def_key;
>> }
>>
>> +static inline void signal_power_supply_changed(struct power_supply *psy)
>> +{
>> + if (module_initialized)
>> + power_supply_changed(psy);
>> +}
>> +
>> static int param_set_ac_online(const char *key, const struct kernel_param *kp)
>> {
>> ac_online = map_get_value(map_ac_online, key, ac_online);
>> - power_supply_changed(&test_power_supplies[0]);
>> + signal_power_supply_changed(&test_power_supplies[0]);
>> return 0;
>> }
>>
>> @@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
>> static int param_set_usb_online(const char *key, const struct kernel_param *kp)
>> {
>> usb_online = map_get_value(map_ac_online, key, usb_online);
>> - power_supply_changed(&test_power_supplies[2]);
>> + signal_power_supply_changed(&test_power_supplies[2]);
>> return 0;
>> }
>>
>> @@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_status = map_get_value(map_status, key, battery_status);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_health = map_get_value(map_health, key, battery_health);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_present = map_get_value(map_present, key, battery_present);
>> - power_supply_changed(&test_power_supplies[0]);
>> + signal_power_supply_changed(&test_power_supplies[0]);
>> return 0;
>> }
>>
>> @@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
>> {
>> battery_technology = map_get_value(map_technology, key,
>> battery_technology);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
>> return -EINVAL;
>>
>> battery_capacity = tmp;
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
>> return -EINVAL;
>>
>> battery_voltage = tmp;
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> --
>> 1.7.9.5
Fixed ... I think.
Thank you,
Andrey
View attachment "test_power.patch" of type "text/x-patch" (4310 bytes)
Powered by blists - more mailing lists