| 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: Thu, 14 Nov 2013 16:32:57 -0600
From: Felipe Contreras <felipe.contreras@...il.com>
To: Levente Kurusa <levex@...ux.com>,
Felipe Contreras <felipe.contreras@...il.com>,
Ingo Molnar <mingo@...nel.org>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] panic: setup panic_timeout early
Hi,
Sending again since the previous one was rejected by the server.
On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
<felipe.contreras@...il.com> wrote:
> Levente Kurusa wrote:
>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>> > On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@...nel.org> wrote:
>> >>
>> >> * Felipe Contreras <felipe.contreras@...il.com> wrote:
>> >>
>> >>> Otherwise we might not reboot when the user needs it the most (early
>> >>> on).
>> >>>
>> >>> Signed-off-by: Felipe Contreras <felipe.contreras@...il.com>
>> >>> ---
>> >>>
>> >> [...]
>> >>>
>> >>> diff --git a/kernel/panic.c b/kernel/panic.c
>> >>> index b6c482c..d865263 100644
>> >>> --- a/kernel/panic.c
>> >>> +++ b/kernel/panic.c
>> >>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>> >>>
>> >>> #endif
>> >>>
>> >>> -core_param(panic, panic_timeout, int, 0644);
>> >>> core_param(pause_on_oops, pause_on_oops, int, 0644);
>> >>>
>> >>> +static int __init set_panic_timeout(char *val)
>> >>> +{
>> >>> + long timeout;
>> >>> + int ret;
>> >>> +
>> >>> + ret = kstrtol(val, 0, &timeout);
>> >>> + if (ret < 0)
>> >>> + return ret;
>> >>> +
>> >>> + panic_timeout = timeout;
>> >>> + return 0;
>> >>> +}
>> >>
>> >> I think the type of the 'timeout' local variable should match the type of
>> >> 'panic_timeout' (which is 'int', not 'long').
>> >
>> > So you would rather have this?
>> >
>> > kstrtol(val, 0, (long *)&timeout);
>> >
>> > Couldn't that potentially write the value beyond the memory allocated
>> > to 'timeout'?
>> >
>>
>> No, 'panic_timeout' is a variable of type 'int'.
>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>
> Oh really? Something like this?
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
> static int __init set_panic_timeout(char *val)
> {
> - long timeout;
> + int timeout;
> int ret;
>
> ret = kstrtol(val, 0, &timeout);
>
> And then what happens?
>
> kernel/panic.c: In function ‘set_panic_timeout’:
> kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from incompatible pointer type [enabled by default]
> ret = kstrtol(val, 0, &timeout);
> ^
> In file included from include/linux/debug_locks.h:4:0,
> from kernel/panic.c:11:
> include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is of type ‘int *’
> static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>
> To fix that warning you need this:
> ^
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
> static int __init set_panic_timeout(char *val)
> {
> - long timeout;
> + int timeout;
> int ret;
>
> - ret = kstrtol(val, 0, &timeout);
> + ret = kstrtol(val, 0, (long *)&timeout);
> if (ret < 0)
> return ret;
>
>
> Which is the only logical conclusion I arrived to. What else do you suggest to
> fix the problem that kstrtol() expects a long? And since this fix is not what
> we want because we would be writing to the wrong memory, we don't want 'timeout'
> to be int.
>
> Therefore 'timeout' should be a long. How is that not clear?
>
> You can even see that it's already done this way for parameters:
>
> STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>
> #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn) \
> int param_set_##name(const char *val, const struct kernel_param *kp) \
> { \
> tmptype l; \
> int ret; \
> \
> ret = strtolfn(val, 0, &l); \
> if (ret < 0 || ((type)l != l)) \
> return ret < 0 ? ret : -EINVAL; \
> *((type *)kp->arg) = l; \
> return 0; \
> } \
>
> So yes, we obviously want the temporary variable 'timeout' to be a long, even
> though the final destination is an int.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists