* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-15 20:15 [PATCH] ceph: Annotate struct ceph_monmap with __counted_by Kees Cook
@ 2023-09-15 20:51 ` Gustavo A. R. Silva
2023-09-18 0:25 ` Xiubo Li
2023-09-18 9:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2023-09-15 20:51 UTC (permalink / raw)
To: Kees Cook, Ilya Dryomov
Cc: Xiubo Li, Jeff Layton, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, ceph-devel, netdev,
Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, llvm,
linux-hardening
On 9/15/23 14: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>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
> ---
> 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;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-15 20:15 [PATCH] ceph: Annotate struct ceph_monmap with __counted_by Kees Cook
2023-09-15 20:51 ` Gustavo A. R. Silva
@ 2023-09-18 0:25 ` Xiubo Li
2023-09-18 2:00 ` Kees Cook
2023-09-18 2:04 ` Gustavo A. R. Silva
2023-09-18 9:40 ` patchwork-bot+netdevbpf
2 siblings, 2 replies; 7+ messages in thread
From: Xiubo Li @ 2023-09-18 0:25 UTC (permalink / raw)
To: Kees Cook, Ilya Dryomov
Cc: Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, ceph-devel, netdev, Nathan Chancellor,
Nick Desaulniers, Tom Rix, linux-kernel, llvm, linux-hardening
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 ?
> return 0;
> }
>
Else LGTM.
Reviewed-by: Xiubo Li <xiubli@redhat.com>
Thanks!
- Xiubo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-18 0:25 ` Xiubo Li
@ 2023-09-18 2:00 ` Kees Cook
2023-09-18 2:04 ` Gustavo A. R. Silva
1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2023-09-18 2:00 UTC (permalink / raw)
To: Xiubo Li, Kees Cook, Ilya Dryomov
Cc: Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, ceph-devel, netdev, Nathan Chancellor,
Nick Desaulniers, Tom Rix, linux-kernel, llvm, linux-hardening
On September 17, 2023 5:25:10 PM PDT, Xiubo Li <xiubli@redhat.com> wrote:
>
>On 9/16/23 04:15, Kees Cook wrote:
> > [...]
>> Additionally, since the element count member must be set before accessing
>> the annotated flexible array member, move its initialization earlier.
>>
>> [...]
>> 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, this is needed so that the __counted_by size is set before accessing the flexible array.
>
>> return 0;
>> }
>>
>
>Else LGTM.
>
>Reviewed-by: Xiubo Li <xiubli@redhat.com>
Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-18 0:25 ` Xiubo Li
2023-09-18 2:00 ` Kees Cook
@ 2023-09-18 2:04 ` Gustavo A. R. Silva
2023-09-18 2:25 ` Xiubo Li
1 sibling, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2023-09-18 2:04 UTC (permalink / raw)
To: Xiubo Li, Kees Cook, Ilya Dryomov
Cc: Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, ceph-devel, netdev, Nathan Chancellor,
Nick Desaulniers, Tom Rix, linux-kernel, llvm, linux-hardening
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`.
--
Gustavo
>
>> return 0;
>> }
>
> Else LGTM.
>
> Reviewed-by: Xiubo Li <xiubli@redhat.com>
>
> Thanks!
>
> - Xiubo
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-18 2:04 ` Gustavo A. R. Silva
@ 2023-09-18 2:25 ` Xiubo Li
0 siblings, 0 replies; 7+ messages in thread
From: Xiubo Li @ 2023-09-18 2:25 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kees Cook, Ilya Dryomov
Cc: Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, ceph-devel, netdev, Nathan Chancellor,
Nick Desaulniers, Tom Rix, linux-kernel, llvm, linux-hardening
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
>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: Annotate struct ceph_monmap with __counted_by
2023-09-15 20:15 [PATCH] ceph: Annotate struct ceph_monmap with __counted_by Kees Cook
2023-09-15 20:51 ` Gustavo A. R. Silva
2023-09-18 0:25 ` Xiubo Li
@ 2023-09-18 9:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-18 9:40 UTC (permalink / raw)
To: Kees Cook
Cc: idryomov, xiubli, jlayton, davem, edumazet, kuba, pabeni,
ceph-devel, netdev, nathan, ndesaulniers, trix, linux-kernel,
llvm, linux-hardening
Hello:
This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:
On Fri, 15 Sep 2023 13:15:10 -0700 you 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.
>
> [...]
Here is the summary with links:
- ceph: Annotate struct ceph_monmap with __counted_by
https://git.kernel.org/netdev/net-next/c/1cb6422ecac8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 7+ messages in thread