for EXPORT_SYMBOL_GPL functions, necessary to check their parameters.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/kallsyms.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 2169fee..4ba57a9 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -175,6 +175,9 @@ unsigned long kallsyms_lookup_name(const char *name)
unsigned long i;
unsigned int off;
+ if (!name || !name[0])
+ return 0;
+
for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
off = kallsyms_expand_symbol(off, namebuf);
@@ -194,6 +197,9 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
unsigned int off;
int ret;
+ if (!fn)
+ return -EINVAL;
+
for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
off = kallsyms_expand_symbol(off, namebuf);
ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
@@ -382,6 +388,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
*/
int sprint_symbol(char *buffer, unsigned long address)
{
+ if (!buffer)
+ return 0;
+
return __sprint_symbol(buffer, address, 0, 1);
}
EXPORT_SYMBOL_GPL(sprint_symbol);
@@ -399,6 +408,9 @@ EXPORT_SYMBOL_GPL(sprint_symbol);
*/
int sprint_symbol_no_offset(char *buffer, unsigned long address)
{
+ if (!buffer)
+ return 0;
+
return __sprint_symbol(buffer, address, 0, 0);
}
EXPORT_SYMBOL_GPL(sprint_symbol_no_offset);
--
1.7.7.6
Chen Gang <[email protected]> writes:
> for EXPORT_SYMBOL_GPL functions, necessary to check their parameters.
>
> Signed-off-by: Chen Gang <[email protected]>
Why?
If someone misuses these functions, they crash and thus indicate that
the caller shouldn't do that.
Or is someone already doing this?
Confused,
Rusty.
> ---
> kernel/kallsyms.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 2169fee..4ba57a9 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -175,6 +175,9 @@ unsigned long kallsyms_lookup_name(const char *name)
> unsigned long i;
> unsigned int off;
>
> + if (!name || !name[0])
> + return 0;
> +
> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> off = kallsyms_expand_symbol(off, namebuf);
>
> @@ -194,6 +197,9 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
> unsigned int off;
> int ret;
>
> + if (!fn)
> + return -EINVAL;
> +
> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> off = kallsyms_expand_symbol(off, namebuf);
> ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
> @@ -382,6 +388,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
> */
> int sprint_symbol(char *buffer, unsigned long address)
> {
> + if (!buffer)
> + return 0;
> +
> return __sprint_symbol(buffer, address, 0, 1);
> }
> EXPORT_SYMBOL_GPL(sprint_symbol);
> @@ -399,6 +408,9 @@ EXPORT_SYMBOL_GPL(sprint_symbol);
> */
> int sprint_symbol_no_offset(char *buffer, unsigned long address)
> {
> + if (!buffer)
> + return 0;
> +
> return __sprint_symbol(buffer, address, 0, 0);
> }
> EXPORT_SYMBOL_GPL(sprint_symbol_no_offset);
> --
> 1.7.7.6
On 2013年04月10日 14:57, Rusty Russell wrote:
> Chen Gang <[email protected]> writes:
>> > for EXPORT_SYMBOL_GPL functions, necessary to check their parameters.
>> >
>> > Signed-off-by: Chen Gang <[email protected]>
> Why?
>
> If someone misuses these functions, they crash and thus indicate that
> the caller shouldn't do that.
>
for me, I think:
if it is used by self (such as static functions):
I prefer to crash immediatly.
it will help us to find issue, quickly.
if it can be used by others (such as EXPORT_SYMBOL_GPL):
I prefer to return fail and tell caller that parameter is invalid.
it is more polite to callers, and still indicate it may be an issue.
:-)
> Or is someone already doing this?
>
really has:
kernel: __wake_up_sync_key in kernel/sched/core.c.
lib: *printf.
mm: kfree.
> Confused,
> Rusty.
>
--
Chen Gang
Asianux Corporation
Chen Gang <[email protected]> writes:
> On 2013年04月10日 14:57, Rusty Russell wrote:
>> Chen Gang <[email protected]> writes:
>>> > for EXPORT_SYMBOL_GPL functions, necessary to check their parameters.
>>> >
>>> > Signed-off-by: Chen Gang <[email protected]>
>> Why?
>>
>> If someone misuses these functions, they crash and thus indicate that
>> the caller shouldn't do that.
>>
>
> for me, I think:
>
> if it is used by self (such as static functions):
> I prefer to crash immediatly.
> it will help us to find issue, quickly.
>
> if it can be used by others (such as EXPORT_SYMBOL_GPL):
> I prefer to return fail and tell caller that parameter is invalid.
> it is more polite to callers, and still indicate it may be an issue.
>
> :-)
I disagree. Calling with invalid parameters is a bug. You've just
covered up some cases of invalid use and made it less likely to be
found. Because the caller won't notice they screwed up.
We could sprinkle WARN_ON() everywhere, but I prefer the crash. Even
harder to ignore.
There's no limit to how many of these checks we could put in, and we can
*never* take them out. I don't want to code that way.
>> Or is someone already doing this?
>>
>
> really has:
>
> kernel: __wake_up_sync_key in kernel/sched/core.c.
> lib: *printf.
> mm: kfree.
No, I mean "is someone calling these functions with NULL".
Cheers,
Rusty.
On 2013年04月11日 10:52, Rusty Russell wrote:
>>> >> Or is someone already doing this?
>>> >>
>> >
>> > really has:
>> >
>> > kernel: __wake_up_sync_key in kernel/sched/core.c.
>> > lib: *printf.
>> > mm: kfree.
> No, I mean "is someone calling these functions with NULL".
>
> Cheers,
> Rusty.
>
>
often "kfree(NULL)", that is ok. it will let caller easier using it.
also often give 0 size to snprintf, it still let caller easy using.
if we treat EXPORT functions of kallsyms as commonly used (or we want to)
I suggest to give parameter check for them.
thanks.
:-)
--
Chen Gang
Asianux Corporation