| Message ID | 20230922175319.work.096-kees@kernel.org |
|---|---|
| State | Accepted |
| Commit | c66650d29764e228eba40b7a59fdb70fa6567daa |
| Headers | show |
| Series | cxl/acpi: Annotate struct cxl_cxims_data with __counted_by | expand |
On 9/22/23 10:53, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: linux-cxl@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/acpi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index d1c559879dcc..40d055560e52 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -14,7 +14,7 @@ > > struct cxl_cxims_data { > int nr_maps; > - u64 xormaps[]; > + u64 xormaps[] __counted_by(nr_maps); > }; > > /* > @@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, > GFP_KERNEL); > if (!cximsd) > return -ENOMEM; > + cximsd->nr_maps = nr_maps; > memcpy(cximsd->xormaps, cxims->xormap_list, > nr_maps * sizeof(*cximsd->xormaps)); > - cximsd->nr_maps = nr_maps; > cxlrd->platform_data = cximsd; > > return 0;
On Fri, 2023-09-22 at 10:53 -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: linux-cxl@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/cxl/acpi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index d1c559879dcc..40d055560e52 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -14,7 +14,7 @@ > > struct cxl_cxims_data { > int nr_maps; > - u64 xormaps[]; > + u64 xormaps[] __counted_by(nr_maps); > }; > > /* > @@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, > GFP_KERNEL); > if (!cximsd) > return -ENOMEM; > + cximsd->nr_maps = nr_maps; > memcpy(cximsd->xormaps, cxims->xormap_list, > nr_maps * sizeof(*cximsd->xormaps)); > - cximsd->nr_maps = nr_maps; > cxlrd->platform_data = cximsd; > > return 0;
On Fri, Sep 22, 2023 at 10:53:19AM -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: linux-cxl@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > drivers/cxl/acpi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index d1c559879dcc..40d055560e52 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -14,7 +14,7 @@ > > struct cxl_cxims_data { > int nr_maps; > - u64 xormaps[]; > + u64 xormaps[] __counted_by(nr_maps); > }; > > /* > @@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, > GFP_KERNEL); > if (!cximsd) > return -ENOMEM; > + cximsd->nr_maps = nr_maps; > memcpy(cximsd->xormaps, cxims->xormap_list, > nr_maps * sizeof(*cximsd->xormaps)); > - cximsd->nr_maps = nr_maps; > cxlrd->platform_data = cximsd; > > return 0; > -- > 2.34.1 > >
On Fri, 22 Sep 2023, Kees Cook wrote: >Prepare for the coming implementation by GCC and Clang of the __counted_by >attribute. Flexible array members annotated with __counted_by can have >their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS >(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family >functions). > >As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. >Additionally, since the element count member must be set before accessing >the annotated flexible array member, move its initialization earlier. Nice. Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> > >[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > >Cc: Davidlohr Bueso <dave@stgolabs.net> >Cc: Jonathan Cameron <jonathan.cameron@huawei.com> >Cc: Dave Jiang <dave.jiang@intel.com> >Cc: Alison Schofield <alison.schofield@intel.com> >Cc: Vishal Verma <vishal.l.verma@intel.com> >Cc: Ira Weiny <ira.weiny@intel.com> >Cc: Dan Williams <dan.j.williams@intel.com> >Cc: linux-cxl@vger.kernel.org >Signed-off-by: Kees Cook <keescook@chromium.org> >--- > drivers/cxl/acpi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c >index d1c559879dcc..40d055560e52 100644 >--- a/drivers/cxl/acpi.c >+++ b/drivers/cxl/acpi.c >@@ -14,7 +14,7 @@ > > struct cxl_cxims_data { > int nr_maps; >- u64 xormaps[]; >+ u64 xormaps[] __counted_by(nr_maps); > }; > > /* >@@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, > GFP_KERNEL); > if (!cximsd) > return -ENOMEM; >+ cximsd->nr_maps = nr_maps; > memcpy(cximsd->xormaps, cxims->xormap_list, > nr_maps * sizeof(*cximsd->xormaps)); >- cximsd->nr_maps = nr_maps; > cxlrd->platform_data = cximsd; > > return 0; >-- >2.34.1 >
On Sat, Sep 23, 2023 at 2:53 AM Kees Cook <keescook@chromium.org> wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: linux-cxl@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Looks good. The reordering of the count assignment is crucial here otherwise the runtime checks will trip when `cximsd->xormaps` is used as memcpy destination. Reviewed-by: Justin Stitt <justinstitt@google.com> > --- > drivers/cxl/acpi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index d1c559879dcc..40d055560e52 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -14,7 +14,7 @@ > > struct cxl_cxims_data { > int nr_maps; > - u64 xormaps[]; > + u64 xormaps[] __counted_by(nr_maps); > }; > > /* > @@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, > GFP_KERNEL); > if (!cximsd) > return -ENOMEM; > + cximsd->nr_maps = nr_maps; > memcpy(cximsd->xormaps, cxims->xormap_list, > nr_maps * sizeof(*cximsd->xormaps)); > - cximsd->nr_maps = nr_maps; > cxlrd->platform_data = cximsd; > > return 0; > -- > 2.34.1 > >
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index d1c559879dcc..40d055560e52 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -14,7 +14,7 @@ struct cxl_cxims_data { int nr_maps; - u64 xormaps[]; + u64 xormaps[] __counted_by(nr_maps); }; /* @@ -112,9 +112,9 @@ static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg, GFP_KERNEL); if (!cximsd) return -ENOMEM; + cximsd->nr_maps = nr_maps; memcpy(cximsd->xormaps, cxims->xormap_list, nr_maps * sizeof(*cximsd->xormaps)); - cximsd->nr_maps = nr_maps; cxlrd->platform_data = cximsd; return 0;
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct cxl_cxims_data. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Davidlohr Bueso <dave@stgolabs.net> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Alison Schofield <alison.schofield@intel.com> Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: linux-cxl@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/cxl/acpi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)