| Message ID | Y3Jd5iZRbNQ9w6gm@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drbd: use after free in drbd_create_device() | expand |
Am 15.11.22 um 14:16 schrieb Dan Carpenter: > The drbd_destroy_connection() frees the "connection" so use the _safe() > iterator to prevent a use after free. > > Fixes: b6f85ef9538b ("drbd: Iterate over all connections") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > Smatch assumes that kref_put() generally calls the free function so it > gets very confused by drbd_delete_device() which calls: > > kref_put(&device->kref, drbd_destroy_device); > > Four times in a row. (Smatch has some checking for incremented > reference counts but even there it assumes that people are going to hold > one reference and not four). > > drivers/block/drbd/drbd_main.c:2831 drbd_delete_device() error: dereferencing freed memory 'device' > drivers/block/drbd/drbd_main.c:2833 drbd_delete_device() warn: passing freed memory 'device' > drivers/block/drbd/drbd_main.c:2835 drbd_delete_device() error: dereferencing freed memory 'device' > > The drbd_adm_get_status_all() function makes me itch as well. It seems > like we drop a reference and then take it again? > > drivers/block/drbd/drbd_nl.c:4019 drbd_adm_get_status_all() warn: 'resource' was already freed. > drivers/block/drbd/drbd_nl.c:4021 drbd_adm_get_status_all() warn: 'resource' was already freed. > > drivers/block/drbd/drbd_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c > index f3e4db16fd07..8532b839a343 100644 > --- a/drivers/block/drbd/drbd_main.c > +++ b/drivers/block/drbd/drbd_main.c > @@ -2672,7 +2672,7 @@ static int init_submitter(struct drbd_device *device) > enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsigned int minor) > { > struct drbd_resource *resource = adm_ctx->resource; > - struct drbd_connection *connection; > + struct drbd_connection *connection, *n; > struct drbd_device *device; > struct drbd_peer_device *peer_device, *tmp_peer_device; > struct gendisk *disk; > @@ -2789,7 +2789,7 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsig > return NO_ERROR; > > out_idr_remove_from_resource: > - for_each_connection(connection, resource) { > + for_each_connection_safe(connection, n, resource) { > peer_device = idr_remove(&connection->peer_devices, vnr); > if (peer_device) > kref_put(&connection->kref, drbd_destroy_connection); Thanks! Reviewed-by: Christoph Böhmwalder <christoph.boehmwalder@linbit.com>
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index f3e4db16fd07..8532b839a343 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -2672,7 +2672,7 @@ static int init_submitter(struct drbd_device *device) enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsigned int minor) { struct drbd_resource *resource = adm_ctx->resource; - struct drbd_connection *connection; + struct drbd_connection *connection, *n; struct drbd_device *device; struct drbd_peer_device *peer_device, *tmp_peer_device; struct gendisk *disk; @@ -2789,7 +2789,7 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsig return NO_ERROR; out_idr_remove_from_resource: - for_each_connection(connection, resource) { + for_each_connection_safe(connection, n, resource) { peer_device = idr_remove(&connection->peer_devices, vnr); if (peer_device) kref_put(&connection->kref, drbd_destroy_connection);
The drbd_destroy_connection() frees the "connection" so use the _safe() iterator to prevent a use after free. Fixes: b6f85ef9538b ("drbd: Iterate over all connections") Signed-off-by: Dan Carpenter <error27@gmail.com> --- Smatch assumes that kref_put() generally calls the free function so it gets very confused by drbd_delete_device() which calls: kref_put(&device->kref, drbd_destroy_device); Four times in a row. (Smatch has some checking for incremented reference counts but even there it assumes that people are going to hold one reference and not four). drivers/block/drbd/drbd_main.c:2831 drbd_delete_device() error: dereferencing freed memory 'device' drivers/block/drbd/drbd_main.c:2833 drbd_delete_device() warn: passing freed memory 'device' drivers/block/drbd/drbd_main.c:2835 drbd_delete_device() error: dereferencing freed memory 'device' The drbd_adm_get_status_all() function makes me itch as well. It seems like we drop a reference and then take it again? drivers/block/drbd/drbd_nl.c:4019 drbd_adm_get_status_all() warn: 'resource' was already freed. drivers/block/drbd/drbd_nl.c:4021 drbd_adm_get_status_all() warn: 'resource' was already freed. drivers/block/drbd/drbd_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)