| Message ID | 86c522f5e01e438b4a9cc16a0bda87a207d744e6.1698666319.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix error pointer dereference after failure to allocate fs devices | expand |
On 2023/10/30 22:24, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At device_list_add() we allocate a btrfs_fs_devices structure and then > before checking if the allocation failed (pointer is ERR_PTR(-ENOMEM)), > we dereference the error pointer in a memcpy() argument if the feature > BTRFS_FEATURE_INCOMPAT_METADATA_UUID is enabled. > Fix this by checking for an allocation error before trying the memcpy(). > > Fixes: f7361d8c3fc3 ("btrfs: sipmlify uuid parameters of alloc_fs_devices()") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/volumes.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1fdfa9153e30..dd279241f78c 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -746,13 +746,13 @@ static noinline struct btrfs_device *device_list_add(const char *path, > > if (!fs_devices) { > fs_devices = alloc_fs_devices(disk_super->fsid); > + if (IS_ERR(fs_devices)) > + return ERR_CAST(fs_devices); > + > if (has_metadata_uuid) > memcpy(fs_devices->metadata_uuid, > disk_super->metadata_uuid, BTRFS_FSID_SIZE); > > - if (IS_ERR(fs_devices)) > - return ERR_CAST(fs_devices); > - > if (same_fsid_diff_dev) { > generate_random_uuid(fs_devices->fsid); > fs_devices->temp_fsid = true;
On 10/30/23 19:54, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At device_list_add() we allocate a btrfs_fs_devices structure and then > before checking if the allocation failed (pointer is ERR_PTR(-ENOMEM)), > we dereference the error pointer in a memcpy() argument if the feature > BTRFS_FEATURE_INCOMPAT_METADATA_UUID is enabled. > Fix this by checking for an allocation error before trying the memcpy(). > > Fixes: f7361d8c3fc3 ("btrfs: sipmlify uuid parameters of alloc_fs_devices()") > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/volumes.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1fdfa9153e30..dd279241f78c 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -746,13 +746,13 @@ static noinline struct btrfs_device *device_list_add(const char *path, > > if (!fs_devices) { > fs_devices = alloc_fs_devices(disk_super->fsid); > + if (IS_ERR(fs_devices)) > + return ERR_CAST(fs_devices); > + > if (has_metadata_uuid) > memcpy(fs_devices->metadata_uuid, > disk_super->metadata_uuid, BTRFS_FSID_SIZE); > > - if (IS_ERR(fs_devices)) > - return ERR_CAST(fs_devices); > - Aiyo! Thank you for the fix. How were you able to identify this issue? Reviewed-by: Anand Jain <anand.jain@oracle.com> > if (same_fsid_diff_dev) { > generate_random_uuid(fs_devices->fsid); > fs_devices->temp_fsid = true;
On Tue, Oct 31, 2023 at 1:12 AM Anand Jain <anand.jain@oracle.com> wrote: > > On 10/30/23 19:54, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At device_list_add() we allocate a btrfs_fs_devices structure and then > > before checking if the allocation failed (pointer is ERR_PTR(-ENOMEM)), > > we dereference the error pointer in a memcpy() argument if the feature > > BTRFS_FEATURE_INCOMPAT_METADATA_UUID is enabled. > > Fix this by checking for an allocation error before trying the memcpy(). > > > > Fixes: f7361d8c3fc3 ("btrfs: sipmlify uuid parameters of alloc_fs_devices()") > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/volumes.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 1fdfa9153e30..dd279241f78c 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -746,13 +746,13 @@ static noinline struct btrfs_device *device_list_add(const char *path, > > > > if (!fs_devices) { > > fs_devices = alloc_fs_devices(disk_super->fsid); > > + if (IS_ERR(fs_devices)) > > + return ERR_CAST(fs_devices); > > + > > if (has_metadata_uuid) > > memcpy(fs_devices->metadata_uuid, > > disk_super->metadata_uuid, BTRFS_FSID_SIZE); > > > > - if (IS_ERR(fs_devices)) > > - return ERR_CAST(fs_devices); > > - > > Aiyo! > > Thank you for the fix. How were you able to identify this issue? By reading code while working on a large change in volumes.c... > > Reviewed-by: Anand Jain <anand.jain@oracle.com> > > > > > if (same_fsid_diff_dev) { > > generate_random_uuid(fs_devices->fsid); > > fs_devices->temp_fsid = true; >
On Mon, Oct 30, 2023 at 11:54:23AM +0000, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At device_list_add() we allocate a btrfs_fs_devices structure and then > before checking if the allocation failed (pointer is ERR_PTR(-ENOMEM)), > we dereference the error pointer in a memcpy() argument if the feature > BTRFS_FEATURE_INCOMPAT_METADATA_UUID is enabled. > Fix this by checking for an allocation error before trying the memcpy(). > > Fixes: f7361d8c3fc3 ("btrfs: sipmlify uuid parameters of alloc_fs_devices()") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 1fdfa9153e30..dd279241f78c 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -746,13 +746,13 @@ static noinline struct btrfs_device *device_list_add(const char *path, if (!fs_devices) { fs_devices = alloc_fs_devices(disk_super->fsid); + if (IS_ERR(fs_devices)) + return ERR_CAST(fs_devices); + if (has_metadata_uuid) memcpy(fs_devices->metadata_uuid, disk_super->metadata_uuid, BTRFS_FSID_SIZE); - if (IS_ERR(fs_devices)) - return ERR_CAST(fs_devices); - if (same_fsid_diff_dev) { generate_random_uuid(fs_devices->fsid); fs_devices->temp_fsid = true;