[PATCH 0/2] smb: client: improve copy_ref_data() robustness

[PATCH 0/2] smb: client: improve copy_ref_data() robustness

[Fredric Cover]

 1. Avoid setting the metadata of the cache
    before updating the cache.

 2. Harden copy_ref_data() against an empty ref
    array.

Fredric Cover (2):
  smb: client: fill cache fields after populating cache in
    copy_ref_data()
  smb: client: prevent out-of-bounds dfs_info3_param access in
    copy_ref_data()

 fs/smb/client/dfs_cache.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH 1/2] smb: client: fill cache fields after populating cache in copy_ref_data()

[Fredric Cover]

In copy_ref_data(), struct cache_entry *ce has its fields populated
at the beginning of the function. Later, if alloc_target fails with
an ERR_PTR, free_tgts() is called on the cache, leaving the cache
metadata populated without any targets. Critically, this extends
ce->etime, making the cache appear valid for longer without any
targets.

Also, free_tgts() does not set ce->numtgts to zero. On error, when
the cache is freed, ce->numtgts is not zeroed, and other cache users
may attempt to access nonexistent entries.

Update fields after copying targets to prevent partial-state updates.
Set ce->numtgts to zero at the end of free_tgts().

Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
---
 fs/smb/client/dfs_cache.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
index 86dba25b7a5a..b0388c460499 100644
--- a/fs/smb/client/dfs_cache.c
+++ b/fs/smb/client/dfs_cache.c
@@ -123,6 +123,7 @@ static inline void free_tgts(struct cache_entry *ce)
 		kfree(t);
 	}
 
+	ce->numtgts = 0;
 	WRITE_ONCE(ce->tgthint, NULL);
 }
 
@@ -388,13 +389,6 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
 	struct cache_dfs_tgt *target;
 	int i;
 
-	ce->ttl = max_t(int, refs[0].ttl, CACHE_MIN_TTL);
-	ce->etime = get_expire_time(ce->ttl);
-	ce->srvtype = refs[0].server_type;
-	ce->hdr_flags = refs[0].flags;
-	ce->ref_flags = refs[0].ref_flag;
-	ce->path_consumed = refs[0].path_consumed;
-
 	for (i = 0; i < numrefs; i++) {
 		struct cache_dfs_tgt *t;
 
@@ -409,12 +403,19 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
 		} else {
 			list_add_tail(&t->list, &ce->tlist);
 		}
-		ce->numtgts++;
 	}
 
 	target = list_first_entry_or_null(&ce->tlist, struct cache_dfs_tgt,
 					  list);
+
 	WRITE_ONCE(ce->tgthint, target);
+	ce->ttl = max_t(int, refs[0].ttl, CACHE_MIN_TTL);
+	ce->etime = get_expire_time(ce->ttl);
+	ce->srvtype = refs[0].server_type;
+	ce->hdr_flags = refs[0].flags;
+	ce->ref_flags = refs[0].ref_flag;
+	ce->path_consumed = refs[0].path_consumed;
+	ce->numtgts = numrefs;
 
 	return 0;
 }
-- 
2.53.0

Re: [PATCH 1/2] smb: client: fill cache fields after populating cache in copy_ref_data()

[Paulo Alcantara]

Fredric Cover <fredric.cover.lkernel@gmail.com> writes:

> In copy_ref_data(), struct cache_entry *ce has its fields populated
> at the beginning of the function. Later, if alloc_target fails with
> an ERR_PTR, free_tgts() is called on the cache, leaving the cache
> metadata populated without any targets. Critically, this extends
> ce->etime, making the cache appear valid for longer without any
> targets.
>
> Also, free_tgts() does not set ce->numtgts to zero. On error, when
> the cache is freed, ce->numtgts is not zeroed, and other cache users
> may attempt to access nonexistent entries.
>
> Update fields after copying targets to prevent partial-state updates.
> Set ce->numtgts to zero at the end of free_tgts().
>
> Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
> ---
>  fs/smb/client/dfs_cache.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
> index 86dba25b7a5a..b0388c460499 100644
> --- a/fs/smb/client/dfs_cache.c
> +++ b/fs/smb/client/dfs_cache.c
> @@ -123,6 +123,7 @@ static inline void free_tgts(struct cache_entry *ce)
>  		kfree(t);
>  	}
>  
> +	ce->numtgts = 0;

Since you're now setting ce->numtgts to 0 in free_tgts(), you could get
rid of the same setting in update_cache_entry_locked() after free_tgts()
is called.

The rest looks good.

[PATCH 2/2] smb: client: prevent out-of-bounds dfs_info3_param access in copy_ref_data()

[Fredric Cover]

Currently, copy_ref_data() assumes that numrefs > 0, and unconditinally
accesses refs[0]. If copy_ref_data() somehow is passed an empty list,
this would cause major out-of-bounds write and read problems.

Enforce numrefs > 0.

Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
---
 fs/smb/client/dfs_cache.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
index b0388c460499..b51c8355d73d 100644
--- a/fs/smb/client/dfs_cache.c
+++ b/fs/smb/client/dfs_cache.c
@@ -389,6 +389,9 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
 	struct cache_dfs_tgt *target;
 	int i;
 
+	if (WARN_ON_ONCE(numrefs <= 0))
+		return -EINVAL;
+
 	for (i = 0; i < numrefs; i++) {
 		struct cache_dfs_tgt *t;
 
-- 
2.53.0

Re: [PATCH 2/2] smb: client: prevent out-of-bounds dfs_info3_param access in copy_ref_data()

[Paulo Alcantara]

Fredric Cover <fredric.cover.lkernel@gmail.com> writes:

> Currently, copy_ref_data() assumes that numrefs > 0, and unconditinally
> accesses refs[0]. If copy_ref_data() somehow is passed an empty list,
> this would cause major out-of-bounds write and read problems.
>
> Enforce numrefs > 0.
>
> Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
> ---
>  fs/smb/client/dfs_cache.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
> index b0388c460499..b51c8355d73d 100644
> --- a/fs/smb/client/dfs_cache.c
> +++ b/fs/smb/client/dfs_cache.c
> @@ -389,6 +389,9 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
>  	struct cache_dfs_tgt *target;
>  	int i;
>  
> +	if (WARN_ON_ONCE(numrefs <= 0))
> +		return -EINVAL;

I don't understand why this check is necessary.  All target referrals
come from get_dfs_referral(), and in case @numrefs < 0,
parse_dfs_referrals() would have returned -ENOENT and copy_ref_data()
wouldn't be called.

Re: [PATCH 2/2] smb: client: prevent out-of-bounds dfs_info3_param access in copy_ref_data()

[Paulo Alcantara]

Paulo Alcantara <pc@manguebit.org> writes:

> Fredric Cover <fredric.cover.lkernel@gmail.com> writes:
>
>> Currently, copy_ref_data() assumes that numrefs > 0, and unconditinally
>> accesses refs[0]. If copy_ref_data() somehow is passed an empty list,
>> this would cause major out-of-bounds write and read problems.
>>
>> Enforce numrefs > 0.
>>
>> Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
>> ---
>>  fs/smb/client/dfs_cache.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
>> index b0388c460499..b51c8355d73d 100644
>> --- a/fs/smb/client/dfs_cache.c
>> +++ b/fs/smb/client/dfs_cache.c
>> @@ -389,6 +389,9 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
>>  	struct cache_dfs_tgt *target;
>>  	int i;
>>  
>> +	if (WARN_ON_ONCE(numrefs <= 0))
>> +		return -EINVAL;
>
> I don't understand why this check is necessary.  All target referrals
> come from get_dfs_referral(), and in case @numrefs < 0,
                                            ^ @numrefs <= 0                                        

Sorry for the typo.

Re: [PATCH 2/2] smb: client: prevent out-of-bounds dfs_info3_param access in copy_ref_data()

[Fredric Cover]

Hi Paulo,

That makes sense. I originally added the check as purely defensive programming
because refs[0] is accessed unconditionally at the end of the function, and I
wanted to guard against potential future caller bugs.

However, as you noted, parse_dfs_referrals() already guarantees that numrefs > 0
on success.

I'll drop patch 2/2 and send patch 1/2 as a standalone v2 shortly, which will
also include the removal of the redundant ce->numtgts assignment in
update_cache_entry_locked().

Thanks,
Fredric



On Wed, Sep 2, 2026 at 10:48 AM Paulo Alcantara <pc@manguebit.org> wrote:
>
> Paulo Alcantara <pc@manguebit.org> writes:
>
> > Fredric Cover <fredric.cover.lkernel@gmail.com> writes:
> >
> >> Currently, copy_ref_data() assumes that numrefs > 0, and unconditinally
> >> accesses refs[0]. If copy_ref_data() somehow is passed an empty list,
> >> this would cause major out-of-bounds write and read problems.
> >>
> >> Enforce numrefs > 0.
> >>
> >> Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
> >> ---
> >>  fs/smb/client/dfs_cache.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
> >> index b0388c460499..b51c8355d73d 100644
> >> --- a/fs/smb/client/dfs_cache.c
> >> +++ b/fs/smb/client/dfs_cache.c
> >> @@ -389,6 +389,9 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
> >>      struct cache_dfs_tgt *target;
> >>      int i;
> >>
> >> +    if (WARN_ON_ONCE(numrefs <= 0))
> >> +            return -EINVAL;
> >
> > I don't understand why this check is necessary.  All target referrals
> > come from get_dfs_referral(), and in case @numrefs < 0,
>                                             ^ @numrefs <= 0
>
> Sorry for the typo.