| Message ID | 4904f32e-a392-803b-9766-4aa2d5cee12b@omp.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | NFSv4: prevent integer overflow while calling nfs4_set_lease_period() | expand |
Hi Sergey, On 8/23/24 4:32 PM, Sergey Shtylyov wrote: > The nfs_client::cl_lease_time field (as well as the jiffies variable it's > used with) is declared as *unsigned long*, which is 32-bit type on 32-bit > arches and 64-bit type on 64-bit arches. When nfs4_set_lease_period() that > sets nfs_client::cl_lease_time is called, 32-bit nfs_fsinfo::lease_time > field is multiplied by HZ -- that might overflow *unsigned long* on 32-bit > arches. Actually, there's no need to multiply by HZ at all the call sites > of nfs4_set_lease_period() -- it makes more sense to do that once, inside > that function (using mul_u32_u32(), as it produces a better code on 32-bit > x86 arch), also checking for an overflow there and returning -ERANGE if it > does happen (we're also making that function *int* instead of *void* and > adding the result checks to its callers)... > > Found by Linux Verification Center (linuxtesting.org) with the Svace static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > Cc: stable@vger.kernel.org > > --- > This patch is against the master branch of Trond Myklebust's linux-nfs.git repo. > > fs/nfs/nfs4_fs.h | 3 +-- > fs/nfs/nfs4proc.c | 3 ++- > fs/nfs/nfs4renewd.c | 13 ++++++++++--- > fs/nfs/nfs4state.c | 4 +++- > 4 files changed, 16 insertions(+), 7 deletions(-) > > Index: linux-nfs/fs/nfs/nfs4_fs.h > =================================================================== > --- linux-nfs.orig/fs/nfs/nfs4_fs.h > +++ linux-nfs/fs/nfs/nfs4_fs.h > @@ -464,8 +464,7 @@ struct nfs_client *nfs4_alloc_client(con > extern void nfs4_schedule_state_renewal(struct nfs_client *); > extern void nfs4_kill_renewd(struct nfs_client *); > extern void nfs4_renew_state(struct work_struct *); > -extern void nfs4_set_lease_period(struct nfs_client *clp, unsigned long lease); > - > +extern int nfs4_set_lease_period(struct nfs_client *clp, u32 period); > > /* nfs4state.c */ > extern const nfs4_stateid current_stateid; > Index: linux-nfs/fs/nfs/nfs4proc.c > =================================================================== > --- linux-nfs.orig/fs/nfs/nfs4proc.c > +++ linux-nfs/fs/nfs/nfs4proc.c > @@ -5403,7 +5403,8 @@ static int nfs4_do_fsinfo(struct nfs_ser > err = _nfs4_do_fsinfo(server, fhandle, fsinfo); > trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err); > if (err == 0) { > - nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ); > + err = nfs4_set_lease_period(server->nfs_client, > + fsinfo->lease_time); > break; > } > err = nfs4_handle_exception(server, err, &exception); > Index: linux-nfs/fs/nfs/nfs4renewd.c > =================================================================== > --- linux-nfs.orig/fs/nfs/nfs4renewd.c > +++ linux-nfs/fs/nfs/nfs4renewd.c > @@ -137,15 +137,22 @@ nfs4_kill_renewd(struct nfs_client *clp) > * nfs4_set_lease_period - Sets the lease period on a nfs_client > * > * @clp: pointer to nfs_client > - * @lease: new value for lease period > + * @period: new value for lease period (in seconds) > */ > -void nfs4_set_lease_period(struct nfs_client *clp, > - unsigned long lease) > +int nfs4_set_lease_period(struct nfs_client *clp, u32 period) > { > + u64 result = mul_u32_u32(period, HZ); Thanks for the patch! Sorry it took a couple of weeks for me to look at. I do like this change for doing the calculation in a single place, rather than relying on callers to do it first. > + unsigned long lease = result; > + > + /* First see if period * HZ actually fits into unsigned long... */ > + if (result > ULONG_MAX) > + return -ERANGE; However, I'm not sold that this should be an error condition. I wonder if it would be better to change the clp->cl_lease_time field to a u64 so it has the same size on 32bit and 64bit architectures? Thanks, Anna > + > spin_lock(&clp->cl_lock); > clp->cl_lease_time = lease; > spin_unlock(&clp->cl_lock); > > /* Cap maximum reconnect timeout at 1/2 lease period */ > rpc_set_connect_timeout(clp->cl_rpcclient, lease, lease >> 1); > + return 0; > } > Index: linux-nfs/fs/nfs/nfs4state.c > =================================================================== > --- linux-nfs.orig/fs/nfs/nfs4state.c > +++ linux-nfs/fs/nfs/nfs4state.c > @@ -103,7 +103,9 @@ static int nfs4_setup_state_renewal(stru > > status = nfs4_proc_get_lease_time(clp, &fsinfo); > if (status == 0) { > - nfs4_set_lease_period(clp, fsinfo.lease_time * HZ); > + status = nfs4_set_lease_period(clp, fsinfo.lease_time); > + if (status) > + return status; > nfs4_schedule_state_renewal(clp); > } >
On 9/16/24 22:44, Anna Schumaker wrote: [...] >> The nfs_client::cl_lease_time field (as well as the jiffies variable it's >> used with) is declared as *unsigned long*, which is 32-bit type on 32-bit >> arches and 64-bit type on 64-bit arches. When nfs4_set_lease_period() that >> sets nfs_client::cl_lease_time is called, 32-bit nfs_fsinfo::lease_time >> field is multiplied by HZ -- that might overflow *unsigned long* on 32-bit >> arches. Actually, there's no need to multiply by HZ at all the call sites >> of nfs4_set_lease_period() -- it makes more sense to do that once, inside >> that function (using mul_u32_u32(), as it produces a better code on 32-bit >> x86 arch), also checking for an overflow there and returning -ERANGE if it >> does happen (we're also making that function *int* instead of *void* and >> adding the result checks to its callers)... >> >> Found by Linux Verification Center (linuxtesting.org) with the Svace static >> analysis tool. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> Cc: stable@vger.kernel.org [...] >> Index: linux-nfs/fs/nfs/nfs4renewd.c >> =================================================================== >> --- linux-nfs.orig/fs/nfs/nfs4renewd.c >> +++ linux-nfs/fs/nfs/nfs4renewd.c >> @@ -137,15 +137,22 @@ nfs4_kill_renewd(struct nfs_client *clp) >> * nfs4_set_lease_period - Sets the lease period on a nfs_client >> * >> * @clp: pointer to nfs_client >> - * @lease: new value for lease period >> + * @period: new value for lease period (in seconds) >> */ >> -void nfs4_set_lease_period(struct nfs_client *clp, >> - unsigned long lease) >> +int nfs4_set_lease_period(struct nfs_client *clp, u32 period) >> { >> + u64 result = mul_u32_u32(period, HZ); > > Thanks for the patch! Sorry it took a couple of weeks for me to look at. NP, better late than never. :-) > I do like this change for doing the calculation in a single place, rather than relying on callers to do it first. > >> + unsigned long lease = result; >> + >> + /* First see if period * HZ actually fits into unsigned long... */ >> + if (result > ULONG_MAX) >> + return -ERANGE; > > However, I'm not sold that this should be an error condition. I wonder if it would be better to change the clp->cl_lease_time field to a u64 so it has the same size on 32bit and 64bit architectures? That would be much more invasive change. And pretty fruitless, I think, because nfs_client::cl_lease_time is only involved in the jiffies math, and since jiffies is *unsigned long* (i.e. 32-bit on a 32-bit arch), the results of our u64 math would get truncated to 32 bits anyway in the end... Looking at time_{after|before}(), I even suspected they could complain at compile time -- however I wasn't able to trigger that so far... > Thanks, > Anna [...] MBR, Sergey
Index: linux-nfs/fs/nfs/nfs4_fs.h =================================================================== --- linux-nfs.orig/fs/nfs/nfs4_fs.h +++ linux-nfs/fs/nfs/nfs4_fs.h @@ -464,8 +464,7 @@ struct nfs_client *nfs4_alloc_client(con extern void nfs4_schedule_state_renewal(struct nfs_client *); extern void nfs4_kill_renewd(struct nfs_client *); extern void nfs4_renew_state(struct work_struct *); -extern void nfs4_set_lease_period(struct nfs_client *clp, unsigned long lease); - +extern int nfs4_set_lease_period(struct nfs_client *clp, u32 period); /* nfs4state.c */ extern const nfs4_stateid current_stateid; Index: linux-nfs/fs/nfs/nfs4proc.c =================================================================== --- linux-nfs.orig/fs/nfs/nfs4proc.c +++ linux-nfs/fs/nfs/nfs4proc.c @@ -5403,7 +5403,8 @@ static int nfs4_do_fsinfo(struct nfs_ser err = _nfs4_do_fsinfo(server, fhandle, fsinfo); trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err); if (err == 0) { - nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ); + err = nfs4_set_lease_period(server->nfs_client, + fsinfo->lease_time); break; } err = nfs4_handle_exception(server, err, &exception); Index: linux-nfs/fs/nfs/nfs4renewd.c =================================================================== --- linux-nfs.orig/fs/nfs/nfs4renewd.c +++ linux-nfs/fs/nfs/nfs4renewd.c @@ -137,15 +137,22 @@ nfs4_kill_renewd(struct nfs_client *clp) * nfs4_set_lease_period - Sets the lease period on a nfs_client * * @clp: pointer to nfs_client - * @lease: new value for lease period + * @period: new value for lease period (in seconds) */ -void nfs4_set_lease_period(struct nfs_client *clp, - unsigned long lease) +int nfs4_set_lease_period(struct nfs_client *clp, u32 period) { + u64 result = mul_u32_u32(period, HZ); + unsigned long lease = result; + + /* First see if period * HZ actually fits into unsigned long... */ + if (result > ULONG_MAX) + return -ERANGE; + spin_lock(&clp->cl_lock); clp->cl_lease_time = lease; spin_unlock(&clp->cl_lock); /* Cap maximum reconnect timeout at 1/2 lease period */ rpc_set_connect_timeout(clp->cl_rpcclient, lease, lease >> 1); + return 0; } Index: linux-nfs/fs/nfs/nfs4state.c =================================================================== --- linux-nfs.orig/fs/nfs/nfs4state.c +++ linux-nfs/fs/nfs/nfs4state.c @@ -103,7 +103,9 @@ static int nfs4_setup_state_renewal(stru status = nfs4_proc_get_lease_time(clp, &fsinfo); if (status == 0) { - nfs4_set_lease_period(clp, fsinfo.lease_time * HZ); + status = nfs4_set_lease_period(clp, fsinfo.lease_time); + if (status) + return status; nfs4_schedule_state_renewal(clp); }
The nfs_client::cl_lease_time field (as well as the jiffies variable it's used with) is declared as *unsigned long*, which is 32-bit type on 32-bit arches and 64-bit type on 64-bit arches. When nfs4_set_lease_period() that sets nfs_client::cl_lease_time is called, 32-bit nfs_fsinfo::lease_time field is multiplied by HZ -- that might overflow *unsigned long* on 32-bit arches. Actually, there's no need to multiply by HZ at all the call sites of nfs4_set_lease_period() -- it makes more sense to do that once, inside that function (using mul_u32_u32(), as it produces a better code on 32-bit x86 arch), also checking for an overflow there and returning -ERANGE if it does happen (we're also making that function *int* instead of *void* and adding the result checks to its callers)... Found by Linux Verification Center (linuxtesting.org) with the Svace static analysis tool. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> Cc: stable@vger.kernel.org --- This patch is against the master branch of Trond Myklebust's linux-nfs.git repo. fs/nfs/nfs4_fs.h | 3 +-- fs/nfs/nfs4proc.c | 3 ++- fs/nfs/nfs4renewd.c | 13 ++++++++++--- fs/nfs/nfs4state.c | 4 +++- 4 files changed, 16 insertions(+), 7 deletions(-)