| Message ID | 20210727205855.411487-49-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
On 7/27/21 1:58 PM, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memset(), avoid intentionally writing across > neighboring fields. > > Add a struct_group() for the algs so that memset() can correctly reason > about the size. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/block/drbd/drbd_main.c | 3 ++- > drivers/block/drbd/drbd_protocol.h | 6 ++++-- > drivers/block/drbd/drbd_receiver.c | 3 ++- > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c > index 55234a558e98..b824679cfcb2 100644 > --- a/drivers/block/drbd/drbd_main.c > +++ b/drivers/block/drbd/drbd_main.c > @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) > cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; > > /* initialize verify_alg and csums_alg */ > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > + memset(&p->algs, 0, sizeof(p->algs)); > > if (get_ldev(peer_device->device)) { > dc = rcu_dereference(peer_device->device->ldev->disk_conf); > diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h > index dea59c92ecc1..a882b65ab5d2 100644 > --- a/drivers/block/drbd/drbd_protocol.h > +++ b/drivers/block/drbd/drbd_protocol.h > @@ -283,8 +283,10 @@ struct p_rs_param_89 { > > struct p_rs_param_95 { > u32 resync_rate; > - char verify_alg[SHARED_SECRET_MAX]; > - char csums_alg[SHARED_SECRET_MAX]; > + struct_group(algs, > + char verify_alg[SHARED_SECRET_MAX]; > + char csums_alg[SHARED_SECRET_MAX]; > + ); > u32 c_plan_ahead; > u32 c_delay_target; > u32 c_fill_target; > diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c > index 1f740e42e457..6df2539e215b 100644 > --- a/drivers/block/drbd/drbd_receiver.c > +++ b/drivers/block/drbd/drbd_receiver.c > @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i > > /* initialize verify_alg and csums_alg */ > p = pi->data; > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > + memset(&p->algs, 0, sizeof(p->algs)); Using struct_group() introduces complexity. Has it been considered not to modify struct p_rs_param_95 and instead to use two memset() calls instead of one (one memset() call per member)? Thanks, Bart.
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote: > On 7/27/21 1:58 PM, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Add a struct_group() for the algs so that memset() can correctly reason > > about the size. > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > drivers/block/drbd/drbd_main.c | 3 ++- > > drivers/block/drbd/drbd_protocol.h | 6 ++++-- > > drivers/block/drbd/drbd_receiver.c | 3 ++- > > 3 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c > > index 55234a558e98..b824679cfcb2 100644 > > --- a/drivers/block/drbd/drbd_main.c > > +++ b/drivers/block/drbd/drbd_main.c > > @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) > > cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; > > /* initialize verify_alg and csums_alg */ > > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > > + memset(&p->algs, 0, sizeof(p->algs)); > > if (get_ldev(peer_device->device)) { > > dc = rcu_dereference(peer_device->device->ldev->disk_conf); > > diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h > > index dea59c92ecc1..a882b65ab5d2 100644 > > --- a/drivers/block/drbd/drbd_protocol.h > > +++ b/drivers/block/drbd/drbd_protocol.h > > @@ -283,8 +283,10 @@ struct p_rs_param_89 { > > struct p_rs_param_95 { > > u32 resync_rate; > > - char verify_alg[SHARED_SECRET_MAX]; > > - char csums_alg[SHARED_SECRET_MAX]; > > + struct_group(algs, > > + char verify_alg[SHARED_SECRET_MAX]; > > + char csums_alg[SHARED_SECRET_MAX]; > > + ); > > u32 c_plan_ahead; > > u32 c_delay_target; > > u32 c_fill_target; > > diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c > > index 1f740e42e457..6df2539e215b 100644 > > --- a/drivers/block/drbd/drbd_receiver.c > > +++ b/drivers/block/drbd/drbd_receiver.c > > @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i > > /* initialize verify_alg and csums_alg */ > > p = pi->data; > > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > > + memset(&p->algs, 0, sizeof(p->algs)); > > Using struct_group() introduces complexity. Has it been considered not to > modify struct p_rs_param_95 and instead to use two memset() calls instead of > one (one memset() call per member)? I went this direction because using two memset()s (or memcpy()s in other patches) changes the machine code. It's not much of a change, but it seems easier to justify "no binary changes" via the use of struct_group(). If splitting the memset() is preferred, I can totally do that instead. :) -Kees
On 7/29/21 7:31 PM, Kees Cook wrote: > On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote: >> On 7/27/21 1:58 PM, Kees Cook wrote: >>> In preparation for FORTIFY_SOURCE performing compile-time and run-time >>> field bounds checking for memset(), avoid intentionally writing across >>> neighboring fields. >>> >>> Add a struct_group() for the algs so that memset() can correctly reason >>> about the size. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> drivers/block/drbd/drbd_main.c | 3 ++- >>> drivers/block/drbd/drbd_protocol.h | 6 ++++-- >>> drivers/block/drbd/drbd_receiver.c | 3 ++- >>> 3 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c >>> index 55234a558e98..b824679cfcb2 100644 >>> --- a/drivers/block/drbd/drbd_main.c >>> +++ b/drivers/block/drbd/drbd_main.c >>> @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) >>> cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; >>> /* initialize verify_alg and csums_alg */ >>> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); >>> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); >>> + memset(&p->algs, 0, sizeof(p->algs)); >>> if (get_ldev(peer_device->device)) { >>> dc = rcu_dereference(peer_device->device->ldev->disk_conf); >>> diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h >>> index dea59c92ecc1..a882b65ab5d2 100644 >>> --- a/drivers/block/drbd/drbd_protocol.h >>> +++ b/drivers/block/drbd/drbd_protocol.h >>> @@ -283,8 +283,10 @@ struct p_rs_param_89 { >>> struct p_rs_param_95 { >>> u32 resync_rate; >>> - char verify_alg[SHARED_SECRET_MAX]; >>> - char csums_alg[SHARED_SECRET_MAX]; >>> + struct_group(algs, >>> + char verify_alg[SHARED_SECRET_MAX]; >>> + char csums_alg[SHARED_SECRET_MAX]; >>> + ); >>> u32 c_plan_ahead; >>> u32 c_delay_target; >>> u32 c_fill_target; >>> diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c >>> index 1f740e42e457..6df2539e215b 100644 >>> --- a/drivers/block/drbd/drbd_receiver.c >>> +++ b/drivers/block/drbd/drbd_receiver.c >>> @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i >>> /* initialize verify_alg and csums_alg */ >>> p = pi->data; >>> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); >>> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); >>> + memset(&p->algs, 0, sizeof(p->algs)); >> >> Using struct_group() introduces complexity. Has it been considered not to >> modify struct p_rs_param_95 and instead to use two memset() calls instead of >> one (one memset() call per member)? > > I went this direction because using two memset()s (or memcpy()s in other > patches) changes the machine code. It's not much of a change, but it > seems easier to justify "no binary changes" via the use of struct_group(). > > If splitting the memset() is preferred, I can totally do that instead. > :) I don't have a strong opinion about this. Lars, do you want to comment on this patch? Thanks, Bart.
On Thu, Jul 29, 2021 at 07:57:47PM -0700, Bart Van Assche wrote: > On 7/29/21 7:31 PM, Kees Cook wrote: > > On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote: > >> On 7/27/21 1:58 PM, Kees Cook wrote: > >>> In preparation for FORTIFY_SOURCE performing compile-time and run-time > >>> field bounds checking for memset(), avoid intentionally writing across > >>> neighboring fields. > >>> > >>> Add a struct_group() for the algs so that memset() can correctly reason > >>> about the size. > >>> > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > >>> --- > >>> drivers/block/drbd/drbd_main.c | 3 ++- > >>> drivers/block/drbd/drbd_protocol.h | 6 ++++-- > >>> drivers/block/drbd/drbd_receiver.c | 3 ++- > >>> 3 files changed, 8 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c > >>> index 55234a558e98..b824679cfcb2 100644 > >>> --- a/drivers/block/drbd/drbd_main.c > >>> +++ b/drivers/block/drbd/drbd_main.c > >>> @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) > >>> cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; > >>> /* initialize verify_alg and csums_alg */ > >>> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > >>> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > >>> + memset(&p->algs, 0, sizeof(p->algs)); > >>> if (get_ldev(peer_device->device)) { > >>> dc = rcu_dereference(peer_device->device->ldev->disk_conf); > >>> diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h > >>> index dea59c92ecc1..a882b65ab5d2 100644 > >>> --- a/drivers/block/drbd/drbd_protocol.h > >>> +++ b/drivers/block/drbd/drbd_protocol.h > >>> @@ -283,8 +283,10 @@ struct p_rs_param_89 { > >>> struct p_rs_param_95 { > >>> u32 resync_rate; > >>> - char verify_alg[SHARED_SECRET_MAX]; > >>> - char csums_alg[SHARED_SECRET_MAX]; > >>> + struct_group(algs, > >>> + char verify_alg[SHARED_SECRET_MAX]; > >>> + char csums_alg[SHARED_SECRET_MAX]; > >>> + ); > >>> u32 c_plan_ahead; > >>> u32 c_delay_target; > >>> u32 c_fill_target; > >>> diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c > >>> index 1f740e42e457..6df2539e215b 100644 > >>> --- a/drivers/block/drbd/drbd_receiver.c > >>> +++ b/drivers/block/drbd/drbd_receiver.c > >>> @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i > >>> /* initialize verify_alg and csums_alg */ > >>> p = pi->data; > >>> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > >>> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > >>> + memset(&p->algs, 0, sizeof(p->algs)); > >> > >> Using struct_group() introduces complexity. Has it been considered not to > >> modify struct p_rs_param_95 and instead to use two memset() calls instead of > >> one (one memset() call per member)? > > > > I went this direction because using two memset()s (or memcpy()s in other > > patches) changes the machine code. It's not much of a change, but it > > seems easier to justify "no binary changes" via the use of struct_group(). > > > > If splitting the memset() is preferred, I can totally do that instead. > > :) > > I don't have a strong opinion about this. Lars, do you want to comment > on this patch? Fine either way. "no binary changes" sounds good ;-) Thanks, Lars
On Thu, Jul 29, 2021 at 7:31 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote: > > On 7/27/21 1:58 PM, Kees Cook wrote: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > field bounds checking for memset(), avoid intentionally writing across > > > neighboring fields. > > > > > > Add a struct_group() for the algs so that memset() can correctly reason > > > about the size. > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > drivers/block/drbd/drbd_main.c | 3 ++- > > > drivers/block/drbd/drbd_protocol.h | 6 ++++-- > > > drivers/block/drbd/drbd_receiver.c | 3 ++- > > > 3 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c > > > index 55234a558e98..b824679cfcb2 100644 > > > --- a/drivers/block/drbd/drbd_main.c > > > +++ b/drivers/block/drbd/drbd_main.c > > > @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) > > > cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; > > > /* initialize verify_alg and csums_alg */ > > > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > > > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > > > + memset(&p->algs, 0, sizeof(p->algs)); > > > if (get_ldev(peer_device->device)) { > > > dc = rcu_dereference(peer_device->device->ldev->disk_conf); > > > diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h > > > index dea59c92ecc1..a882b65ab5d2 100644 > > > --- a/drivers/block/drbd/drbd_protocol.h > > > +++ b/drivers/block/drbd/drbd_protocol.h > > > @@ -283,8 +283,10 @@ struct p_rs_param_89 { > > > struct p_rs_param_95 { > > > u32 resync_rate; > > > - char verify_alg[SHARED_SECRET_MAX]; > > > - char csums_alg[SHARED_SECRET_MAX]; > > > + struct_group(algs, > > > + char verify_alg[SHARED_SECRET_MAX]; > > > + char csums_alg[SHARED_SECRET_MAX]; > > > + ); > > > u32 c_plan_ahead; > > > u32 c_delay_target; > > > u32 c_fill_target; > > > diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c > > > index 1f740e42e457..6df2539e215b 100644 > > > --- a/drivers/block/drbd/drbd_receiver.c > > > +++ b/drivers/block/drbd/drbd_receiver.c > > > @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i > > > /* initialize verify_alg and csums_alg */ > > > p = pi->data; > > > - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); > > > + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); > > > + memset(&p->algs, 0, sizeof(p->algs)); > > > > Using struct_group() introduces complexity. Has it been considered not to > > modify struct p_rs_param_95 and instead to use two memset() calls instead of > > one (one memset() call per member)? > > I went this direction because using two memset()s (or memcpy()s in other > patches) changes the machine code. It's not much of a change, but it > seems easier to justify "no binary changes" via the use of struct_group(). > > If splitting the memset() is preferred, I can totally do that instead. > :) I'm not sure that compilers can fold memsets of adjacent members. It might not matter, but you could wrap these members in a _named_ struct then simply use assignment for optimal codegen.
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index 55234a558e98..b824679cfcb2 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device) cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM; /* initialize verify_alg and csums_alg */ - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); + memset(&p->algs, 0, sizeof(p->algs)); if (get_ldev(peer_device->device)) { dc = rcu_dereference(peer_device->device->ldev->disk_conf); diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h index dea59c92ecc1..a882b65ab5d2 100644 --- a/drivers/block/drbd/drbd_protocol.h +++ b/drivers/block/drbd/drbd_protocol.h @@ -283,8 +283,10 @@ struct p_rs_param_89 { struct p_rs_param_95 { u32 resync_rate; - char verify_alg[SHARED_SECRET_MAX]; - char csums_alg[SHARED_SECRET_MAX]; + struct_group(algs, + char verify_alg[SHARED_SECRET_MAX]; + char csums_alg[SHARED_SECRET_MAX]; + ); u32 c_plan_ahead; u32 c_delay_target; u32 c_fill_target; diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c index 1f740e42e457..6df2539e215b 100644 --- a/drivers/block/drbd/drbd_receiver.c +++ b/drivers/block/drbd/drbd_receiver.c @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i /* initialize verify_alg and csums_alg */ p = pi->data; - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX); + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX); + memset(&p->algs, 0, sizeof(p->algs)); err = drbd_recv_all(peer_device->connection, p, header_size); if (err)
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Add a struct_group() for the algs so that memset() can correctly reason about the size. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/block/drbd/drbd_main.c | 3 ++- drivers/block/drbd/drbd_protocol.h | 6 ++++-- drivers/block/drbd/drbd_receiver.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-)