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 <[email protected]>
Cc: Xiubo Li <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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;
}
--
2.34.1
On September 17, 2023 5:25:10 PM PDT, Xiubo Li <[email protected]> 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 <[email protected]>
Thanks!
--
Kees Cook
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 <[email protected]>
>> Cc: Xiubo Li <[email protected]>
>> Cc: Jeff Layton <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Eric Dumazet <[email protected]>
>> Cc: Jakub Kicinski <[email protected]>
>> Cc: Paolo Abeni <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> 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 <[email protected]>
>
> Thanks!
>
> - Xiubo
>
>
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 <[email protected]>
> Cc: Xiubo Li <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
- Xiubo
Hello:
This patch was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:
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
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 <[email protected]>
>>> Cc: Xiubo Li <[email protected]>
>>> Cc: Jeff Layton <[email protected]>
>>> Cc: "David S. Miller" <[email protected]>
>>> Cc: Eric Dumazet <[email protected]>
>>> Cc: Jakub Kicinski <[email protected]>
>>> Cc: Paolo Abeni <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Kees Cook <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>> Thanks!
>>
>> - Xiubo
>>
>>
>