* [PATCH] ramoops: add pdata NULL check to ramoops_probe
@ 2016-09-28 10:32 Geliang Tang
2016-09-28 19:45 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Geliang Tang @ 2016-09-28 10:32 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: Geliang Tang, linux-kernel
When the pdata is NULL, ramoops_probe() segfaults. So this patch adds
a NULL check to it.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
fs/pstore/ram.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6ad831b..dd9832d 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -576,6 +576,9 @@ static int ramoops_probe(struct platform_device *pdev)
if (cxt->max_dump_cnt)
goto fail_out;
+ if (!pdata)
+ goto fail_out;
+
if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
!pdata->ftrace_size && !pdata->pmsg_size)) {
pr_err("The memory size and the record/console size must be "
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ramoops: add pdata NULL check to ramoops_probe
2016-09-28 10:32 [PATCH] ramoops: add pdata NULL check to ramoops_probe Geliang Tang
@ 2016-09-28 19:45 ` Kees Cook
2016-09-29 10:05 ` Geliang Tang
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2016-09-28 19:45 UTC (permalink / raw)
To: Geliang Tang; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML
On Wed, Sep 28, 2016 at 3:32 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> When the pdata is NULL, ramoops_probe() segfaults. So this patch adds
> a NULL check to it.
While I don't mind the check, is this even possible? A device
triggering a ramoops probe should already have a platform_data
(excepting the DT case which is already covered). Is there a situation
you can create to trigger this Oops?
-Kees
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> fs/pstore/ram.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 6ad831b..dd9832d 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -576,6 +576,9 @@ static int ramoops_probe(struct platform_device *pdev)
> if (cxt->max_dump_cnt)
> goto fail_out;
>
> + if (!pdata)
> + goto fail_out;
> +
> if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> !pdata->ftrace_size && !pdata->pmsg_size)) {
> pr_err("The memory size and the record/console size must be "
> --
> 2.7.4
>
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ramoops: add pdata NULL check to ramoops_probe
2016-09-28 19:45 ` Kees Cook
@ 2016-09-29 10:05 ` Geliang Tang
2016-11-16 0:31 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Geliang Tang @ 2016-09-29 10:05 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, Geliang Tang, linux-kernel
On Wed, Sep 28, 2016 at 12:45:13PM -0700, Kees Cook wrote:
> On Wed, Sep 28, 2016 at 3:32 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> > When the pdata is NULL, ramoops_probe() segfaults. So this patch adds
> > a NULL check to it.
>
> While I don't mind the check, is this even possible? A device
> triggering a ramoops probe should already have a platform_data
> (excepting the DT case which is already covered). Is there a situation
> you can create to trigger this Oops?
>
Hi Kees,
This did happen on my device recently.
We use platform_device_register() or platform_device_register_data() to set the
ramoops_platform_data into dev->platform_data. If this ramoops_platform_data did
not set successfully or if we pass a NULL parameter to platform_device_register(),
we will get a NULL pdata in ramoops_probe(). This will trigger a kernel Oops.
Here is a test for this. If we set dummy_data to NULL in ramoops_register_dummy(),
we will get the Oops:
@@ -747,6 +755,7 @@ static void ramoops_register_dummy(void)
*/
dummy_data->ecc_info.ecc_size = ramoops_ecc == 1 ? 16 : ramoops_ecc;
+ dummy_data = NULL;
dummy = platform_device_register_data(NULL, "ramoops", -1,
dummy_data, sizeof(struct ramoops_platform_data));
if (IS_ERR(dummy)) {
So I think this pdata NULL check is useful.
Thanks.
-Geliang
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > fs/pstore/ram.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 6ad831b..dd9832d 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -576,6 +576,9 @@ static int ramoops_probe(struct platform_device *pdev)
> > if (cxt->max_dump_cnt)
> > goto fail_out;
> >
> > + if (!pdata)
> > + goto fail_out;
> > +
> > if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> > !pdata->ftrace_size && !pdata->pmsg_size)) {
> > pr_err("The memory size and the record/console size must be "
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ramoops: add pdata NULL check to ramoops_probe
2016-09-29 10:05 ` Geliang Tang
@ 2016-11-16 0:31 ` Kees Cook
0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2016-11-16 0:31 UTC (permalink / raw)
To: Geliang Tang; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML
On Thu, Sep 29, 2016 at 3:05 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> On Wed, Sep 28, 2016 at 12:45:13PM -0700, Kees Cook wrote:
>> On Wed, Sep 28, 2016 at 3:32 AM, Geliang Tang <geliangtang@gmail.com> wrote:
>> > When the pdata is NULL, ramoops_probe() segfaults. So this patch adds
>> > a NULL check to it.
>>
>> While I don't mind the check, is this even possible? A device
>> triggering a ramoops probe should already have a platform_data
>> (excepting the DT case which is already covered). Is there a situation
>> you can create to trigger this Oops?
>>
>
> Hi Kees,
>
> This did happen on my device recently.
>
> We use platform_device_register() or platform_device_register_data() to set the
> ramoops_platform_data into dev->platform_data. If this ramoops_platform_data did
> not set successfully or if we pass a NULL parameter to platform_device_register(),
> we will get a NULL pdata in ramoops_probe(). This will trigger a kernel Oops.
>
> Here is a test for this. If we set dummy_data to NULL in ramoops_register_dummy(),
> we will get the Oops:
>
> @@ -747,6 +755,7 @@ static void ramoops_register_dummy(void)
> */
> dummy_data->ecc_info.ecc_size = ramoops_ecc == 1 ? 16 : ramoops_ecc;
>
> + dummy_data = NULL;
> dummy = platform_device_register_data(NULL, "ramoops", -1,
> dummy_data, sizeof(struct ramoops_platform_data));
> if (IS_ERR(dummy)) {
>
> So I think this pdata NULL check is useful.
Fair enough. :) I've adjusted the patch a bit with some additional
pr_err()s added. It should appear in -next shortly.
Thanks!
-Kees
>
> Thanks.
>
> -Geliang
>
>> >
>> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> > ---
>> > fs/pstore/ram.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> > index 6ad831b..dd9832d 100644
>> > --- a/fs/pstore/ram.c
>> > +++ b/fs/pstore/ram.c
>> > @@ -576,6 +576,9 @@ static int ramoops_probe(struct platform_device *pdev)
>> > if (cxt->max_dump_cnt)
>> > goto fail_out;
>> >
>> > + if (!pdata)
>> > + goto fail_out;
>> > +
>> > if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
>> > !pdata->ftrace_size && !pdata->pmsg_size)) {
>> > pr_err("The memory size and the record/console size must be "
>> > --
>> > 2.7.4
>> >
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-16 0:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-28 10:32 [PATCH] ramoops: add pdata NULL check to ramoops_probe Geliang Tang
2016-09-28 19:45 ` Kees Cook
2016-09-29 10:05 ` Geliang Tang
2016-11-16 0:31 ` Kees Cook
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).