| Message ID | 20191204151454.29253-1-aaptel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] cifs: fix possible uninitialized access and race on iface_list | expand |
tentatively merged into cifs-2.6.git for-next and buildbot github tree pending testing On Wed, Dec 4, 2019 at 9:59 AM Paulo Alcantara <pc@cjr.nz> wrote: > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > On December 4, 2019 12:14:54 PM GMT-03:00, Aurelien Aptel <aaptel@suse.com> wrote: >> >> iface[0] was accessed regardless of the count value and without >> locking. >> >> * check count before accessing any ifaces >> * make copy of iface list (it's a simple POD array) and use it without >> locking. >> >> Signed-off-by: Aurelien Aptel <aaptel@suse.com> >> ________________________________ >> changes since v1: >> * remove unecessary kfree() >> >> fs/cifs/sess.c | 29 ++++++++++++++++++++++++++--- >> 1 file changed, 26 insertions(+), 3 deletions(-) >> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c >> index fb3bdc44775c..396400cf2800 100644 >> --- a/fs/cifs/sess.c >> +++ b/fs/cifs/sess.c >> @@ -77,6 +77,8 @@ int cifs_try_adding_channels(struct cifs_ses *ses) >> int i = 0; >> int rc = 0; >> int tries = 0; >> + struct cifs_server_iface *ifaces = NULL; >> + size_t iface_count; >> >> if (left <= 0) { >> cifs_dbg(FYI, >> @@ -90,6 +92,26 @@ int cifs_try_adding_channels(struct cifs_ses *ses) >> return 0; >> } >> >> + /* >> + * Make a copy of the iface list at the time and use that >> + * instead so as to not hold the iface spinlock for opening >> + * channels >> + */ >> + spin_lock(&ses->iface_lock); >> + iface_count = ses->iface_count; >> + if (iface_count <= 0) { >> + spin_unlock(&ses->iface_lock); >> + cifs_dbg(FYI, "no iface list available to open channels\n"); >> + return 0; >> + } >> + ifaces = kmemdup(ses->iface_list, iface_count*sizeof(*ifaces), >> + GFP_ATOMIC); >> + if (!ifaces) { >> + spin_unlock(&ses->iface_lock); >> + return 0; >> + } >> + spin_unlock(&ses->iface_lock); >> + >> /* >> * Keep connecting to same, fastest, iface for all channels as >> * long as its RSS. Try next fastest one if not RSS or channel >> @@ -105,9 +127,9 @@ int cifs_try_adding_channels(struct cifs_ses *ses) >> break; >> } >> >> - iface = &ses->iface_list[i]; >> + iface = &ifaces[i]; >> if (is_ses_using_iface(ses, iface) && !iface->rss_capable) { >> - i = (i+1) % ses->iface_count; >> + i = (i+1) % iface_count; >> continue; >> } >> >> @@ -115,7 +137,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) >> if (rc) { >> cifs_dbg(FYI, "failed to open extra channel on iface#%d rc=%d\n", >> i, rc); >> - i = (i+1) % ses->iface_count; >> + i = (i+1) % iface_count; >> continue; >> } >> >> @@ -124,6 +146,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) >> left--; >> } >> >> + kfree(ifaces); >> return ses->chan_count - old_chan_count; >> } >> > > > -- > Sent from my p≡p for Android.
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index fb3bdc44775c..396400cf2800 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -77,6 +77,8 @@ int cifs_try_adding_channels(struct cifs_ses *ses) int i = 0; int rc = 0; int tries = 0; + struct cifs_server_iface *ifaces = NULL; + size_t iface_count; if (left <= 0) { cifs_dbg(FYI, @@ -90,6 +92,26 @@ int cifs_try_adding_channels(struct cifs_ses *ses) return 0; } + /* + * Make a copy of the iface list at the time and use that + * instead so as to not hold the iface spinlock for opening + * channels + */ + spin_lock(&ses->iface_lock); + iface_count = ses->iface_count; + if (iface_count <= 0) { + spin_unlock(&ses->iface_lock); + cifs_dbg(FYI, "no iface list available to open channels\n"); + return 0; + } + ifaces = kmemdup(ses->iface_list, iface_count*sizeof(*ifaces), + GFP_ATOMIC); + if (!ifaces) { + spin_unlock(&ses->iface_lock); + return 0; + } + spin_unlock(&ses->iface_lock); + /* * Keep connecting to same, fastest, iface for all channels as * long as its RSS. Try next fastest one if not RSS or channel @@ -105,9 +127,9 @@ int cifs_try_adding_channels(struct cifs_ses *ses) break; } - iface = &ses->iface_list[i]; + iface = &ifaces[i]; if (is_ses_using_iface(ses, iface) && !iface->rss_capable) { - i = (i+1) % ses->iface_count; + i = (i+1) % iface_count; continue; } @@ -115,7 +137,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) if (rc) { cifs_dbg(FYI, "failed to open extra channel on iface#%d rc=%d\n", i, rc); - i = (i+1) % ses->iface_count; + i = (i+1) % iface_count; continue; } @@ -124,6 +146,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) left--; } + kfree(ifaces); return ses->chan_count - old_chan_count; }
iface[0] was accessed regardless of the count value and without locking. * check count before accessing any ifaces * make copy of iface list (it's a simple POD array) and use it without locking. Signed-off-by: Aurelien Aptel <aaptel@suse.com> --- changes since v1: * remove unecessary kfree() fs/cifs/sess.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)