* [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