| Message ID | 20170110125643.10990-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
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.
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.
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
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.
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
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.
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
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?
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"
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(-)