Messages in this thread | | | Date | Mon, 18 Sep 2023 10:25:33 +0800 | Subject | Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by | From | Xiubo Li <> |
| |
On 9/18/23 10:04, Gustavo A. R. Silva wrote: > > > On 9/17/23 18:25, Xiubo Li wrote: >> >> On 9/16/23 04:15, 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 ceph_monmap. >>> 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: Ilya Dryomov <idryomov@gmail.com> >>> Cc: Xiubo Li <xiubli@redhat.com> >>> Cc: Jeff Layton <jlayton@kernel.org> >>> Cc: "David S. Miller" <davem@davemloft.net> >>> Cc: Eric Dumazet <edumazet@google.com> >>> Cc: Jakub Kicinski <kuba@kernel.org> >>> Cc: Paolo Abeni <pabeni@redhat.com> >>> Cc: ceph-devel@vger.kernel.org >>> Cc: netdev@vger.kernel.org >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> include/linux/ceph/mon_client.h | 2 +- >>> net/ceph/mon_client.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/linux/ceph/mon_client.h >>> b/include/linux/ceph/mon_client.h >>> index b658961156a0..7a9a40163c0f 100644 >>> --- a/include/linux/ceph/mon_client.h >>> +++ b/include/linux/ceph/mon_client.h >>> @@ -19,7 +19,7 @@ struct ceph_monmap { >>> struct ceph_fsid fsid; >>> u32 epoch; >>> u32 num_mon; >>> - struct ceph_entity_inst mon_inst[]; >>> + struct ceph_entity_inst mon_inst[] __counted_by(num_mon); >>> }; >>> struct ceph_mon_client; >>> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c >>> index faabad6603db..f263f7e91a21 100644 >>> --- a/net/ceph/mon_client.c >>> +++ b/net/ceph/mon_client.c >>> @@ -1136,6 +1136,7 @@ static int build_initial_monmap(struct >>> ceph_mon_client *monc) >>> GFP_KERNEL); >>> if (!monc->monmap) >>> return -ENOMEM; >>> + monc->monmap->num_mon = num_mon; >>> for (i = 0; i < num_mon; i++) { >>> struct ceph_entity_inst *inst = &monc->monmap->mon_inst[i]; >>> @@ -1147,7 +1148,6 @@ static int build_initial_monmap(struct >>> ceph_mon_client *monc) >>> inst->name.type = CEPH_ENTITY_TYPE_MON; >>> inst->name.num = cpu_to_le64(i); >>> } >>> - monc->monmap->num_mon = num_mon; >> >> BTW, is this change related ? > > Yes, it is, and it's described in the changelog text. > > `num_mon` must be updated before the first access to flex-array > `mon_inst`. > Otherwise the compiler cannot properly instrument the code to catch any > out-of-bounds access to `mon_inst`. > Okay, got it.
Thanks
- Xiubo
> -- > Gustavo > >> >>> return 0; >>> } >> >> Else LGTM. >> >> Reviewed-by: Xiubo Li <xiubli@redhat.com> >> >> Thanks! >> >> - Xiubo >> >> >
| |