diff mbox series

ntfs: do not dereference a null ctx on error

Message ID 20230407194433.25659-1-listdansp@mail.ru
State New
Headers
Series ntfs: do not dereference a null ctx on error |

Commit Message

Danila Chernetsov April 7, 2023, 7:44 p.m. UTC 
  In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
'ctx' pointer in error handling.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Danila Chernetsov <listdansp@mail.ru>
---
 fs/ntfs/mft.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Bagas Sanjaya May 3, 2023, 4:16 a.m. UTC | #1 
On Fri, Apr 07, 2023 at 07:44:33PM +0000, Danila Chernetsov wrote:
> In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
> prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
> 'ctx' pointer in error handling.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Duplicate of 10-years-old outstanding patch at [1].

I'm not speaking of the patch itself but rather on unfortunate state
of ntfs subsystem. It seems like the maintainer is MIA (has not
responding to patch submissions for a long time). Some trivial
patches, however, are merged through mm tree.

Konstantin (from newer ntfs3 subsystem), Andrew, would you like to take a
look on this orphaned subsystem (and help reviewing)? I'd like to send
MAINTAINERS update if it turns out to be the case.

Thanks.

[1]: https://lore.kernel.org/all/1358389709-20561-1-git-send-email-nickolai@csail.mit.edu/
Andrew Morton May 3, 2023, 9:06 p.m. UTC | #2 
On Wed, 3 May 2023 11:16:02 +0700 Bagas Sanjaya <bagasdotme@gmail.com> wrote:

> On Fri, Apr 07, 2023 at 07:44:33PM +0000, Danila Chernetsov wrote:
> > In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
> > prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
> > 'ctx' pointer in error handling.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Duplicate of 10-years-old outstanding patch at [1].

Well, the patches are actually quite different.  Is Danila's longer one
better?

> I'm not speaking of the patch itself but rather on unfortunate state
> of ntfs subsystem. It seems like the maintainer is MIA (has not
> responding to patch submissions for a long time). Some trivial
> patches, however, are merged through mm tree.
> 
> Konstantin (from newer ntfs3 subsystem), Andrew, would you like to take a
> look on this orphaned subsystem (and help reviewing)? I'd like to send
> MAINTAINERS update if it turns out to be the case.
> 

Sure, I can join linux-ntfs-dev@lists.sourceforge.net and hendle things
which come along.

Or Christian may want to do that?

> 
> [1]: https://lore.kernel.org/all/1358389709-20561-1-git-send-email-nickolai@csail.mit.edu/
Christian Brauner May 4, 2023, 10:56 a.m. UTC | #3 
On Wed, May 03, 2023 at 02:06:09PM -0700, Andrew Morton wrote:
> On Wed, 3 May 2023 11:16:02 +0700 Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> 
> > On Fri, Apr 07, 2023 at 07:44:33PM +0000, Danila Chernetsov wrote:
> > > In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
> > > prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
> > > 'ctx' pointer in error handling.
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > 
> > Duplicate of 10-years-old outstanding patch at [1].
> 
> Well, the patches are actually quite different.  Is Danila's longer one
> better?
> 
> > I'm not speaking of the patch itself but rather on unfortunate state
> > of ntfs subsystem. It seems like the maintainer is MIA (has not
> > responding to patch submissions for a long time). Some trivial
> > patches, however, are merged through mm tree.
> > 
> > Konstantin (from newer ntfs3 subsystem), Andrew, would you like to take a
> > look on this orphaned subsystem (and help reviewing)? I'd like to send
> > MAINTAINERS update if it turns out to be the case.
> > 
> 
> Sure, I can join linux-ntfs-dev@lists.sourceforge.net and hendle things
> which come along.
> 
> Or Christian may want to do that?

Thanks for asking. I would be able to pick up those patches into
fs.misc. I got an off-list ping about someone also wanting to help
with review apparently.
Namjae Jeon May 5, 2023, 6:17 a.m. UTC | #4 
2023-05-04 6:06 GMT+09:00, Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 3 May 2023 11:16:02 +0700 Bagas Sanjaya <bagasdotme@gmail.com>
> wrote:
>
>> On Fri, Apr 07, 2023 at 07:44:33PM +0000, Danila Chernetsov wrote:
>> > In ntfs_mft_data_extend_allocation_nolock(), if an error condition
>> > occurs
>> > prior to 'ctx' being set to a non-NULL value, avoid dereferencing the
>> > NULL
>> > 'ctx' pointer in error handling.
>> >
>> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Duplicate of 10-years-old outstanding patch at [1].
>
> Well, the patches are actually quite different.  Is Danila's longer one
> better?
Danila's patch is better than old one. old patch doesn't free
allocated cluster in error handling. And Christian will apply this to
his queue if you drop this patch.
>
>> I'm not speaking of the patch itself but rather on unfortunate state
>> of ntfs subsystem. It seems like the maintainer is MIA (has not
>> responding to patch submissions for a long time). Some trivial
>> patches, however, are merged through mm tree.
>>
>> Konstantin (from newer ntfs3 subsystem), Andrew, would you like to take a
>> look on this orphaned subsystem (and help reviewing)? I'd like to send
>> MAINTAINERS update if it turns out to be the case.
>>
>
> Sure, I can join linux-ntfs-dev@lists.sourceforge.net and hendle things
> which come along.
>
> Or Christian may want to do that?
>
>>
>> [1]:
>> https://lore.kernel.org/all/1358389709-20561-1-git-send-email-nickolai@csail.mit.edu/
>
>
>
Namjae Jeon May 5, 2023, 6:20 a.m. UTC | #5 
2023-04-08 4:44 GMT+09:00, Danila Chernetsov <listdansp@mail.ru>:
> In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
> prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
> 'ctx' pointer in error handling.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Danila Chernetsov <listdansp@mail.ru>
> ---
>  fs/ntfs/mft.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 48030899dc6e..e1126ce6f8ec 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -1955,36 +1955,40 @@ static int
> ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
>  				"attribute.%s", es);
>  		NVolSetErrors(vol);
>  	}
> -	a = ctx->attr;
> +	
>  	if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
>  		ntfs_error(vol->sb, "Failed to truncate mft data attribute "
>  				"runlist.%s", es);
>  		NVolSetErrors(vol);
>  	}
> -	if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
> -		if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
> +	if (ctx) {
> +		a = ctx->attr;
> +		if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
> +			if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
>  				a->data.non_resident.mapping_pairs_offset),
>  				old_alen - le16_to_cpu(
> -				a->data.non_resident.mapping_pairs_offset),
> +					a->data.non_resident.mapping_pairs_offset),
>  				rl2, ll, -1, NULL)) {
> -			ntfs_error(vol->sb, "Failed to restore mapping pairs "
> +				ntfs_error(vol->sb, "Failed to restore mapping pairs "
>  					"array.%s", es);
> -			NVolSetErrors(vol);
> -		}
> -		if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> -			ntfs_error(vol->sb, "Failed to restore attribute "
> +				NVolSetErrors(vol);
> +			}
> +			if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> +				ntfs_error(vol->sb, "Failed to restore attribute "
>  					"record.%s", es);
> -			NVolSetErrors(vol);
> +				NVolSetErrors(vol);
> +			}
> +			flush_dcache_mft_record_page(ctx->ntfs_ino);
> +			mark_mft_record_dirty(ctx->ntfs_ino);
>  		}
> -		flush_dcache_mft_record_page(ctx->ntfs_ino);
> -		mark_mft_record_dirty(ctx->ntfs_ino);
> -	} else if (IS_ERR(ctx->mrec)) {
> -		ntfs_error(vol->sb, "Failed to restore attribute search "
> +		else if (IS_ERR(ctx->mrec)) {
> +			ntfs_error(vol->sb, "Failed to restore attribute search "
>  				"context.%s", es);
> -		NVolSetErrors(vol);
> +			NVolSetErrors(vol);
> +		}
> +		if (ctx)
I think that this check is not needed.
> +			ntfs_attr_put_search_ctx(ctx);
>  	}
> -	if (ctx)
> -		ntfs_attr_put_search_ctx(ctx);
>  	if (!IS_ERR(mrec))
>  		unmap_mft_record(mft_ni);
>  	up_write(&mft_ni->runlist.lock);
> --
> 2.25.1
>
>
Namjae Jeon May 6, 2023, 12:52 a.m. UTC | #6 
2023-04-08 4:44 GMT+09:00, Danila Chernetsov <listdansp@mail.ru>:
> In ntfs_mft_data_extend_allocation_nolock(), if an error condition occurs
> prior to 'ctx' being set to a non-NULL value, avoid dereferencing the NULL
> 'ctx' pointer in error handling.
Please check the warnings from checkpatch.pl.

ERROR: trailing whitespace
#107: FILE: fs/ntfs/mft.c:1958:
+^I$

ERROR: "(foo*)" should be "(foo *)"
#118: FILE: fs/ntfs/mft.c:1967:
+			if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(

ERROR: else should follow close brace '}'
#146: FILE: fs/ntfs/mft.c:1984:
 		}
+		else if (IS_ERR(ctx->mrec)) {

>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Danila Chernetsov <listdansp@mail.ru>
> ---
>  fs/ntfs/mft.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 48030899dc6e..e1126ce6f8ec 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -1955,36 +1955,40 @@ static int
> ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
>  				"attribute.%s", es);
>  		NVolSetErrors(vol);
>  	}
> -	a = ctx->attr;
> +	
>  	if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
>  		ntfs_error(vol->sb, "Failed to truncate mft data attribute "
>  				"runlist.%s", es);
>  		NVolSetErrors(vol);
>  	}
> -	if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
> -		if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
> +	if (ctx) {
> +		a = ctx->attr;
> +		if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
> +			if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
>  				a->data.non_resident.mapping_pairs_offset),
>  				old_alen - le16_to_cpu(
> -				a->data.non_resident.mapping_pairs_offset),
> +					a->data.non_resident.mapping_pairs_offset),
>  				rl2, ll, -1, NULL)) {
> -			ntfs_error(vol->sb, "Failed to restore mapping pairs "
> +				ntfs_error(vol->sb, "Failed to restore mapping pairs "
>  					"array.%s", es);
> -			NVolSetErrors(vol);
> -		}
> -		if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> -			ntfs_error(vol->sb, "Failed to restore attribute "
> +				NVolSetErrors(vol);
> +			}
> +			if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> +				ntfs_error(vol->sb, "Failed to restore attribute "
>  					"record.%s", es);
> -			NVolSetErrors(vol);
> +				NVolSetErrors(vol);
> +			}
> +			flush_dcache_mft_record_page(ctx->ntfs_ino);
> +			mark_mft_record_dirty(ctx->ntfs_ino);
>  		}
> -		flush_dcache_mft_record_page(ctx->ntfs_ino);
> -		mark_mft_record_dirty(ctx->ntfs_ino);
> -	} else if (IS_ERR(ctx->mrec)) {
> -		ntfs_error(vol->sb, "Failed to restore attribute search "
> +		else if (IS_ERR(ctx->mrec)) {
> +			ntfs_error(vol->sb, "Failed to restore attribute search "
>  				"context.%s", es);
> -		NVolSetErrors(vol);
> +			NVolSetErrors(vol);
> +		}
> +		if (ctx)
> +			ntfs_attr_put_search_ctx(ctx);
>  	}
> -	if (ctx)
> -		ntfs_attr_put_search_ctx(ctx);
>  	if (!IS_ERR(mrec))
>  		unmap_mft_record(mft_ni);
>  	up_write(&mft_ni->runlist.lock);
> --
> 2.25.1
>
>
diff mbox series

Patch

diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 48030899dc6e..e1126ce6f8ec 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -1955,36 +1955,40 @@  static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
 				"attribute.%s", es);
 		NVolSetErrors(vol);
 	}
-	a = ctx->attr;
+	
 	if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
 		ntfs_error(vol->sb, "Failed to truncate mft data attribute "
 				"runlist.%s", es);
 		NVolSetErrors(vol);
 	}
-	if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
-		if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
+	if (ctx) {
+		a = ctx->attr;
+		if (mp_rebuilt && !IS_ERR(ctx->mrec)) {
+			if (ntfs_mapping_pairs_build(vol, (u8*)a + le16_to_cpu(
 				a->data.non_resident.mapping_pairs_offset),
 				old_alen - le16_to_cpu(
-				a->data.non_resident.mapping_pairs_offset),
+					a->data.non_resident.mapping_pairs_offset),
 				rl2, ll, -1, NULL)) {
-			ntfs_error(vol->sb, "Failed to restore mapping pairs "
+				ntfs_error(vol->sb, "Failed to restore mapping pairs "
 					"array.%s", es);
-			NVolSetErrors(vol);
-		}
-		if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
-			ntfs_error(vol->sb, "Failed to restore attribute "
+				NVolSetErrors(vol);
+			}
+			if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
+				ntfs_error(vol->sb, "Failed to restore attribute "
 					"record.%s", es);
-			NVolSetErrors(vol);
+				NVolSetErrors(vol);
+			}
+			flush_dcache_mft_record_page(ctx->ntfs_ino);
+			mark_mft_record_dirty(ctx->ntfs_ino);
 		}
-		flush_dcache_mft_record_page(ctx->ntfs_ino);
-		mark_mft_record_dirty(ctx->ntfs_ino);
-	} else if (IS_ERR(ctx->mrec)) {
-		ntfs_error(vol->sb, "Failed to restore attribute search "
+		else if (IS_ERR(ctx->mrec)) {
+			ntfs_error(vol->sb, "Failed to restore attribute search "
 				"context.%s", es);
-		NVolSetErrors(vol);
+			NVolSetErrors(vol);
+		}
+		if (ctx)
+			ntfs_attr_put_search_ctx(ctx);
 	}
-	if (ctx)
-		ntfs_attr_put_search_ctx(ctx);
 	if (!IS_ERR(mrec))
 		unmap_mft_record(mft_ni);
 	up_write(&mft_ni->runlist.lock);