[v2,2/2] UBI: Make mtd parameter readable

Submitted by Andy Shevchenko on Jan. 10, 2017, 12:56 p.m.

Details

Message ID 20170110125643.10990-2-andriy.shevchenko@linux.intel.com
State New
Delegated to: Cyrille Pitchen
Headers show

Commit Message

Andy Shevchenko Jan. 10, 2017, 12:56 p.m.
Fix permissions to allow read mtd parameter back (only for owner).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mtd/ubi/build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

richard -rw- weinberger Jan. 10, 2017, 1:33 p.m.
Andy,

On Tue, Jan 10, 2017 at 1:56 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Fix permissions to allow read mtd parameter back (only for owner).
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Please CC all maintainers in future. Otherwise the chance is high that
I'll miss a patch.

> ---
>  drivers/mtd/ubi/build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 54312431750b..7bac790d4298 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1470,7 +1470,7 @@ static int ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
>         return 0;
>  }
>
> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
>  MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024[,ubi_num]]].\n"
>                       "Multiple \"mtd\" parameters may be specified.\n"
>                       "MTD devices may be specified by their number, name, or path to the MTD character device node.\n"

What is the use case?
AFAIKT the permissions are 000 because a parser is involved and to
"understand" the parameter,
a reader needs the ubi_mtd_param_parse() function.
Andy Shevchenko Jan. 10, 2017, 1:48 p.m.
On Tue, 2017-01-10 at 14:33 +0100, Richard Weinberger wrote:
> Andy,
> 
> On Tue, Jan 10, 2017 at 1:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Fix permissions to allow read mtd parameter back (only for owner).
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Please CC all maintainers in future. Otherwise the chance is high that
> I'll miss a patch.

Noted.

> > -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);

> What is the use case?
> AFAIKT the permissions are 000

If it's not 0 in current case than you easily crash the kernel because
parser will be gone at that time. This is fixed by patch 1.

>  because a parser is involved and to
> "understand" the parameter,
> a reader needs the ubi_mtd_param_parse() function.

Are you implying that writer is a bot and reader is human being? 
The use case is obvious (any security reasons are implied?) -- allow
user to see what was written there in the first place.

Permissions 0000 are error prone.
Richard Weinberger Jan. 10, 2017, 1:54 p.m.
Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
>>> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
>>> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
> 
>> What is the use case?
>> AFAIKT the permissions are 000
> 
> If it's not 0 in current case than you easily crash the kernel because
> parser will be gone at that time. This is fixed by patch 1.

Before your changes it was non-issue, right? ;)

> 
>>  because a parser is involved and to
>> "understand" the parameter,
>> a reader needs the ubi_mtd_param_parse() function.
> 
> Are you implying that writer is a bot and reader is human being? 
> The use case is obvious (any security reasons are implied?) -- allow
> user to see what was written there in the first place.

I'm asking for the use case, why is exposing this parameter to user space
a good thing? Who will use it?

> Permissions 0000 are error prone.

Why?

Thanks,
//richard
Andy Shevchenko Jan. 10, 2017, 2:11 p.m.
On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
> Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
> > > > -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> > > > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
> > > What is the use case?
> > > AFAIKT the permissions are 000
> > 
> > If it's not 0 in current case than you easily crash the kernel
> > because
> > parser will be gone at that time. This is fixed by patch 1.
> 
> Before your changes it was non-issue, right? ;)

If annoying section mismatch is not an issue, then yes, correct.

> > >  because a parser is involved and to
> > > "understand" the parameter,
> > > a reader needs the ubi_mtd_param_parse() function.
> > 
> > Are you implying that writer is a bot and reader is human being? 
> > The use case is obvious (any security reasons are implied?) -- allow
> > user to see what was written there in the first place.
> 
> I'm asking for the use case, why is exposing this parameter to user
> space
> a good thing? Who will use it?

Any user. I like the idea to have initial string to be stored somewhere
and visible (imagine the case when it's module and history is gone). It
might be useful for debugging (okay, this case perhaps not anymore, but
in general).

> > Permissions 0000 are error prone.
> 
> Why?

Because of a trick is being used here.

P.S. If you strongly against it I will give up, it doesn't cost my
efforts anymore. That's why one of the possible "solution" is to put
comment there for other brave guys.
Richard Weinberger Jan. 10, 2017, 2:16 p.m.
Am 10.01.2017 um 15:11 schrieb Andy Shevchenko:
> On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
>> Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
>>>>> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
>>>>> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
>>>> What is the use case?
>>>> AFAIKT the permissions are 000
>>>
>>> If it's not 0 in current case than you easily crash the kernel
>>> because
>>> parser will be gone at that time. This is fixed by patch 1.
>>
>> Before your changes it was non-issue, right? ;)
> 
> If annoying section mismatch is not an issue, then yes, correct.

Fixing the section mismatch is a good thing.

>>>>  because a parser is involved and to
>>>> "understand" the parameter,
>>>> a reader needs the ubi_mtd_param_parse() function.
>>>
>>> Are you implying that writer is a bot and reader is human being? 
>>> The use case is obvious (any security reasons are implied?) -- allow
>>> user to see what was written there in the first place.
>>
>> I'm asking for the use case, why is exposing this parameter to user
>> space
>> a good thing? Who will use it?
> 
> Any user. I like the idea to have initial string to be stored somewhere
> and visible (imagine the case when it's module and history is gone). It
> might be useful for debugging (okay, this case perhaps not anymore, but
> in general).
> 
>>> Permissions 0000 are error prone.
>>
>> Why?
> 
> Because of a trick is being used here.

You are not really the answering questions kind of guy? ;-)

> P.S. If you strongly against it I will give up, it doesn't cost my
> efforts anymore. That's why one of the possible "solution" is to put
> comment there for other brave guys.

I'm not at all against it, all I want to know why your patch improves the current
situation what it fixes.

Did you check? Do other drivers in the kernel that have a parser behind a non-trivial
module parameter also expose it to user space?

Thanks,
//richard
Andy Shevchenko Jan. 10, 2017, 2:35 p.m.
On Tue, 2017-01-10 at 15:16 +0100, Richard Weinberger wrote:
> Am 10.01.2017 um 15:11 schrieb Andy Shevchenko:
> > On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
> > > Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:

> > > > Permissions 0000 are error prone.
> > > 
> > > Why?
> > 
> > Because of a trick is being used here.
> 
> You are not really the answering questions kind of guy? ;-)

You see, you have to keep in mind that you are not allowed to use
anything except 0 because parser is gone, variable is gone. I'm talking
about module_param_cb(..., 0) cases only.

> > P.S. If you strongly against it I will give up, it doesn't cost my
> > efforts anymore. That's why one of the possible "solution" is to put
> > comment there for other brave guys.

> Did you check? Do other drivers in the kernel that have a parser
> behind a non-trivial
> module parameter also expose it to user space?

Just checked. The only one which is not, this one.
Richard Weinberger Jan. 10, 2017, 3:31 p.m.
Andy,

Am 10.01.2017 um 15:35 schrieb Andy Shevchenko:
>> Did you check? Do other drivers in the kernel that have a parser
>> behind a non-trivial
>> module parameter also expose it to user space?
> 
> Just checked. The only one which is not, this one.
> 

That's an excellent reason. :-)
But I see also:
mtd/devices/phram.c:module_param_call(phram, phram_param_call, NULL, NULL, 000);

If these two are the only users of 000, let's rip them out to be consistent
with the rest of the kernel.

Thanks,
//richard
Andy Shevchenko Jan. 10, 2017, 3:38 p.m.
On Tue, 2017-01-10 at 16:31 +0100, Richard Weinberger wrote:
> Andy,
> 
> Am 10.01.2017 um 15:35 schrieb Andy Shevchenko:
> > > Did you check? Do other drivers in the kernel that have a parser
> > > behind a non-trivial
> > > module parameter also expose it to user space?
> > 
> > Just checked. The only one which is not, this one.
> > 
> 
> That's an excellent reason. :-)
> But I see also:
> mtd/devices/phram.c:module_param_call(phram, phram_param_call, NULL,
> NULL, 000);
> 
> If these two are the only users of 000, let's rip them out to be
> consistent
> with the rest of the kernel.

Okay, this is full(?) list of those. 3 drivers.

$ git grep -n 'module_param_call(.*, 0\+)' drivers/
drivers/ide/ide.c:277:module_param_call(chs, ide_set_disk_chs, NULL,
NULL, 0);
drivers/ide/ide.c:351:module_param_call(ignore_cable,
ide_set_ignore_cable, NULL, NULL, 0);
drivers/mtd/devices/phram.c:301:module_param_call(phram,
phram_param_call, NULL, NULL, 000);
drivers/platform/x86/thinkpad_acpi.c:9666:      module_param_call(featur
e, set_ibm_param, NULL, NULL, 0); \

$ git grep -n 'module_param_cb(.*, 0\+)' drivers/
drivers/mtd/ubi/block.c:168:module_param_cb(block, &ubiblock_param_ops,
NULL, 0);

I doubt about worth to fix IDE (last time I sent clean up maintainer
told me not to do).

For thinkpad I will look at.

For MTD let's do it now.

So, would you like me to resend with / send separate fix to the other
one?

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 54312431750b..7bac790d4298 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1470,7 +1470,7 @@  static int ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
 	return 0;
 }
 
-module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
+module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
 MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024[,ubi_num]]].\n"
 		      "Multiple \"mtd\" parameters may be specified.\n"
 		      "MTD devices may be specified by their number, name, or path to the MTD character device node.\n"