| Message ID | 20200813043458.165718-1-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/papr_scm: Limit the readability of 'perf_stats' sysfs attribute | expand |
On 8/13/20 10:04 AM, Vaibhav Jain wrote: > The newly introduced 'perf_stats' attribute uses the default access > mode of 0444 letting non-root users access performance stats of an > nvdimm and potentially force the kernel into issuing large number of > expensive HCALLs. Since the information exposed by this attribute > cannot be cached hence its better to ward of access to this attribute > from users who don't need to access these performance statistics. > > Hence this patch adds check in perf_stats_show() to only let users > that are 'perfmon_capable()' to read the nvdimm performance > statistics. > > Fixes: 2d02bf835e573 ('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP') > Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/papr_scm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c > index f439f0dfea7d1..36c51bf8af9a8 100644 > --- a/arch/powerpc/platforms/pseries/papr_scm.c > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > @@ -792,6 +792,10 @@ static ssize_t perf_stats_show(struct device *dev, > struct nvdimm *dimm = to_nvdimm(dev); > struct papr_scm_priv *p = nvdimm_provider_data(dimm); > > + /* Allow access only to perfmon capable users */ > + if (!perfmon_capable()) > + return -EACCES; > + An access check is usually done in open(). This is the read callback IIUC. > if (!p->stat_buffer_len) > return -ENOENT; > > -aneesh
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > On 8/13/20 10:04 AM, Vaibhav Jain wrote: >> The newly introduced 'perf_stats' attribute uses the default access >> mode of 0444 letting non-root users access performance stats of an >> nvdimm and potentially force the kernel into issuing large number of >> expensive HCALLs. Since the information exposed by this attribute >> cannot be cached hence its better to ward of access to this attribute >> from users who don't need to access these performance statistics. >> >> Hence this patch adds check in perf_stats_show() to only let users >> that are 'perfmon_capable()' to read the nvdimm performance >> statistics. >> >> Fixes: 2d02bf835e573 ('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP') >> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/papr_scm.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c >> index f439f0dfea7d1..36c51bf8af9a8 100644 >> --- a/arch/powerpc/platforms/pseries/papr_scm.c >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c >> @@ -792,6 +792,10 @@ static ssize_t perf_stats_show(struct device *dev, >> struct nvdimm *dimm = to_nvdimm(dev); >> struct papr_scm_priv *p = nvdimm_provider_data(dimm); >> >> + /* Allow access only to perfmon capable users */ >> + if (!perfmon_capable()) >> + return -EACCES; >> + > > An access check is usually done in open(). This is the read callback IIUC. Yes. Otherwise an unprivileged user can open the file, and then trick a suid program into reading from it. cheers
Thanks Aneesh and Mpe for reviewing this patch. Michael Ellerman <mpe@ellerman.id.au> writes: > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: [snip] >>> >>> + /* Allow access only to perfmon capable users */ >>> + if (!perfmon_capable()) >>> + return -EACCES; >>> + >> >> An access check is usually done in open(). This is the read callback IIUC. > > Yes. Otherwise an unprivileged user can open the file, and then trick a > suid program into reading from it. Agree, but since the 'open()' for this sysfs attribute is handled by kern-fs, AFAIK dont see any direct way to enforce this policy. Only other way it seems to me is to convert the 'perf_stats' DEVICE_ATTR_RO to DEVICE_ATTR_ADMIN_RO. > > cheers
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c index f439f0dfea7d1..36c51bf8af9a8 100644 --- a/arch/powerpc/platforms/pseries/papr_scm.c +++ b/arch/powerpc/platforms/pseries/papr_scm.c @@ -792,6 +792,10 @@ static ssize_t perf_stats_show(struct device *dev, struct nvdimm *dimm = to_nvdimm(dev); struct papr_scm_priv *p = nvdimm_provider_data(dimm); + /* Allow access only to perfmon capable users */ + if (!perfmon_capable()) + return -EACCES; + if (!p->stat_buffer_len) return -ENOENT;
The newly introduced 'perf_stats' attribute uses the default access mode of 0444 letting non-root users access performance stats of an nvdimm and potentially force the kernel into issuing large number of expensive HCALLs. Since the information exposed by this attribute cannot be cached hence its better to ward of access to this attribute from users who don't need to access these performance statistics. Hence this patch adds check in perf_stats_show() to only let users that are 'perfmon_capable()' to read the nvdimm performance statistics. Fixes: 2d02bf835e573 ('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP') Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- arch/powerpc/platforms/pseries/papr_scm.c | 4 ++++ 1 file changed, 4 insertions(+)