| Message ID | 20220107073407.GG22086@kili (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | ACPI: pfr_telemetry: Fix info leak in pfrt_log_ioctl() | expand |
On Fri, Jan 07, 2022 at 10:34:07AM +0300, Dan Carpenter wrote: > The "data_info" struct is copied to the user. It has a 4 byte struct > hole after the last struct member so we need to memset that to avoid > copying uninitialized stack data to the user. > > Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > When you're adding a new driver to the kernel then please use the new > driver's prefix instead of just the subsystem prefix. > > Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver > Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver > Thanks for pointing this out. > Otherwise it's just up to me to guess what prefix you wanted. > > drivers/acpi/pfr_telemetry.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c > index da50dd80192c..9abf350bd7a5 100644 > --- a/drivers/acpi/pfr_telemetry.c > +++ b/drivers/acpi/pfr_telemetry.c > @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, > union acpi_object *out_obj, in_obj, in_buf; > int ret = -EBUSY; > > + memset(data_info, 0, sizeof(*data_info)); Just one minor question, how about moving above before: data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value; after the sanity check of the _DSM result? thanks, Chenyu > memset(&in_obj, 0, sizeof(in_obj)); > memset(&in_buf, 0, sizeof(in_buf)); > in_obj.type = ACPI_TYPE_PACKAGE;
On Fri, Jan 07, 2022 at 09:46:17PM +0800, Chen Yu wrote: > On Fri, Jan 07, 2022 at 10:34:07AM +0300, Dan Carpenter wrote: > > The "data_info" struct is copied to the user. It has a 4 byte struct > > hole after the last struct member so we need to memset that to avoid > > copying uninitialized stack data to the user. > > > > Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > When you're adding a new driver to the kernel then please use the new > > driver's prefix instead of just the subsystem prefix. > > > > Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver > > Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver > > > Thanks for pointing this out. > > Otherwise it's just up to me to guess what prefix you wanted. > > > > drivers/acpi/pfr_telemetry.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c > > index da50dd80192c..9abf350bd7a5 100644 > > --- a/drivers/acpi/pfr_telemetry.c > > +++ b/drivers/acpi/pfr_telemetry.c > > @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, > > union acpi_object *out_obj, in_obj, in_buf; > > int ret = -EBUSY; > > > > + memset(data_info, 0, sizeof(*data_info)); > Just one minor question, how about moving above before: > data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value; > after the sanity check of the _DSM result? I guess I wanted to keep all the memsets together. I feel like if the data is invalid, then it's going to be a slow path and it's not worth optimizing that case. If the data is invalid then a little slow down is the least of our concerns. regards, dan carpenter
On Mon, Jan 10, 2022 at 7:17 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Fri, Jan 07, 2022 at 09:46:17PM +0800, Chen Yu wrote: > > On Fri, Jan 07, 2022 at 10:34:07AM +0300, Dan Carpenter wrote: > > > The "data_info" struct is copied to the user. It has a 4 byte struct > > > hole after the last struct member so we need to memset that to avoid > > > copying uninitialized stack data to the user. > > > > > > Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > When you're adding a new driver to the kernel then please use the new > > > driver's prefix instead of just the subsystem prefix. > > > > > > Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver > > > Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver > > > > > Thanks for pointing this out. > > > Otherwise it's just up to me to guess what prefix you wanted. > > > > > > drivers/acpi/pfr_telemetry.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c > > > index da50dd80192c..9abf350bd7a5 100644 > > > --- a/drivers/acpi/pfr_telemetry.c > > > +++ b/drivers/acpi/pfr_telemetry.c > > > @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, > > > union acpi_object *out_obj, in_obj, in_buf; > > > int ret = -EBUSY; > > > > > > + memset(data_info, 0, sizeof(*data_info)); > > Just one minor question, how about moving above before: > > data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value; > > after the sanity check of the _DSM result? > > I guess I wanted to keep all the memsets together. I feel like if the > data is invalid, then it's going to be a slow path and it's not worth > optimizing that case. If the data is invalid then a little slow down is > the least of our concerns. Patch applied, thanks! Yu, this series needs to spend a few days more in linux-next, because of the fixes against it sent lately.
On Mon, Jan 10, 2022 at 09:17:13AM +0300, Dan Carpenter wrote: > On Fri, Jan 07, 2022 at 09:46:17PM +0800, Chen Yu wrote: > > On Fri, Jan 07, 2022 at 10:34:07AM +0300, Dan Carpenter wrote: > > > The "data_info" struct is copied to the user. It has a 4 byte struct > > > hole after the last struct member so we need to memset that to avoid > > > copying uninitialized stack data to the user. > > > > > > Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > When you're adding a new driver to the kernel then please use the new > > > driver's prefix instead of just the subsystem prefix. > > > > > > Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver > > > Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver > > > > > Thanks for pointing this out. > > > Otherwise it's just up to me to guess what prefix you wanted. > > > > > > drivers/acpi/pfr_telemetry.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c > > > index da50dd80192c..9abf350bd7a5 100644 > > > --- a/drivers/acpi/pfr_telemetry.c > > > +++ b/drivers/acpi/pfr_telemetry.c > > > @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, > > > union acpi_object *out_obj, in_obj, in_buf; > > > int ret = -EBUSY; > > > > > > + memset(data_info, 0, sizeof(*data_info)); > > Just one minor question, how about moving above before: > > data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value; > > after the sanity check of the _DSM result? > > I guess I wanted to keep all the memsets together. I feel like if the > data is invalid, then it's going to be a slow path and it's not worth > optimizing that case. If the data is invalid then a little slow down is > the least of our concerns. > Ok, got it. thanks, Chenyu >
On Mon, Jan 10, 2022 at 04:39:30PM +0100, Rafael J. Wysocki wrote: > On Mon, Jan 10, 2022 at 7:17 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Fri, Jan 07, 2022 at 09:46:17PM +0800, Chen Yu wrote: > > > On Fri, Jan 07, 2022 at 10:34:07AM +0300, Dan Carpenter wrote: > > > > The "data_info" struct is copied to the user. It has a 4 byte struct > > > > hole after the last struct member so we need to memset that to avoid > > > > copying uninitialized stack data to the user. > > > > > > > > Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > When you're adding a new driver to the kernel then please use the new > > > > driver's prefix instead of just the subsystem prefix. > > > > > > > > Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver > > > > Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver > > > > > > > Thanks for pointing this out. > > > > Otherwise it's just up to me to guess what prefix you wanted. > > > > > > > > drivers/acpi/pfr_telemetry.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c > > > > index da50dd80192c..9abf350bd7a5 100644 > > > > --- a/drivers/acpi/pfr_telemetry.c > > > > +++ b/drivers/acpi/pfr_telemetry.c > > > > @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, > > > > union acpi_object *out_obj, in_obj, in_buf; > > > > int ret = -EBUSY; > > > > > > > > + memset(data_info, 0, sizeof(*data_info)); > > > Just one minor question, how about moving above before: > > > data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value; > > > after the sanity check of the _DSM result? > > > > I guess I wanted to keep all the memsets together. I feel like if the > > data is invalid, then it's going to be a slow path and it's not worth > > optimizing that case. If the data is invalid then a little slow down is > > the least of our concerns. > > Patch applied, thanks! > > Yu, this series needs to spend a few days more in linux-next, because > of the fixes against it sent lately. Ok, I see. thanks, Chenyu
diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c index da50dd80192c..9abf350bd7a5 100644 --- a/drivers/acpi/pfr_telemetry.c +++ b/drivers/acpi/pfr_telemetry.c @@ -83,6 +83,7 @@ static int get_pfrt_log_data_info(struct pfrt_log_data_info *data_info, union acpi_object *out_obj, in_obj, in_buf; int ret = -EBUSY; + memset(data_info, 0, sizeof(*data_info)); memset(&in_obj, 0, sizeof(in_obj)); memset(&in_buf, 0, sizeof(in_buf)); in_obj.type = ACPI_TYPE_PACKAGE;
The "data_info" struct is copied to the user. It has a 4 byte struct hole after the last struct member so we need to memset that to avoid copying uninitialized stack data to the user. Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- When you're adding a new driver to the kernel then please use the new driver's prefix instead of just the subsystem prefix. Bad: ACPI: Introduce Platform Firmware Runtime Telemetry driver Good: ACPI / pfr_telemetry: Introduce Platform Firmware Runtime Telemetry driver Otherwise it's just up to me to guess what prefix you wanted. drivers/acpi/pfr_telemetry.c | 1 + 1 file changed, 1 insertion(+)