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@xxxxxxxxxxxxx> wrote:
>
> Paulo Alcantara <pc@xxxxxxxxxxxxx> writes:
>
> > Fredric Cover <fredric.cover.lkernel@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >>  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.