diff mbox

[v3] fs/fcntl: restore checking against COMPAT_LOFF_T_MAX for F_GETLK64

Message ID 20171114164818.6783-1-lav@etersoft.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Vitaly Lipatov Nov. 14, 2017, 4:48 p.m. UTC
for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX.

Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64"

Signed-off-by: Vitaly Lipatov <lav@etersoft.ru>
---
 fs/fcntl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

J. Bruce Fields Nov. 14, 2017, 5:17 p.m. UTC | #1
On Tue, Nov 14, 2017 at 07:48:18PM +0300, Vitaly Lipatov wrote:
> for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX.
> 
> Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64"
> 
> Signed-off-by: Vitaly Lipatov <lav@etersoft.ru>
> ---
>  fs/fcntl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 30f47d0..e9443d9 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -590,17 +590,17 @@ convert_fcntl_cmd(unsigned int cmd)
>   * GETLK was successful and we need to return the data, but it needs to fit in
>   * the compat structure.
>   * l_start shouldn't be too big, unless the original start + end is greater than

I assume that should be start + end.

> - * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return
> + * off_t_max, in which case the app was asking for trouble, so we return
>   * -EOVERFLOW in that case.

It took me a minute to understand.  OK, I get it, the application's not
supposed to issue a GETLK with offset+len too large, so of course it
shouldn't encounter a conflicting lock out there.

I don't think that's true, though, thanks to the special interpretation
of length 0 in the argument; it looks to me like we can find a conflict
with a lock that starts beyond COMPAT_OFF_T_MAX in that case.

I guess that's independent of your patch, though.

--b.

> l_len could be too big, in which case we just
>   * truncate it, and only allow the app to see that part of the conflicting lock
>   * that might make sense to it anyway
>   */
> -static int fixup_compat_flock(struct flock *flock)
> +static int fixup_compat_flock(struct flock *flock, loff_t off_t_max)
>  {
> -	if (flock->l_start > COMPAT_OFF_T_MAX)
> +	if (flock->l_start > off_t_max)
>  		return -EOVERFLOW;
> -	if (flock->l_len > COMPAT_OFF_T_MAX)
> -		flock->l_len = COMPAT_OFF_T_MAX;
> +	if (flock->l_len > off_t_max)
> +		flock->l_len = off_t_max;
>  	return 0;
>  }
>  
> @@ -631,7 +631,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
>  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>  		if (err)
>  			break;
> -		err = fixup_compat_flock(&flock);
> +		err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX);
>  		if (err)
>  			return err;
>  		err = put_compat_flock(&flock, compat_ptr(arg));
> @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
>  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>  		if (err)
>  			break;
> -		err = fixup_compat_flock(&flock);
> +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
>  		if (err)
>  			return err;
>  		err = put_compat_flock64(&flock, compat_ptr(arg));
> -- 
> 2.10.4
Jeff Layton Nov. 14, 2017, 7:12 p.m. UTC | #2
On Tue, 2017-11-14 at 19:48 +0300, Vitaly Lipatov wrote:
> for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX.
> 
> Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64"
> 
> Signed-off-by: Vitaly Lipatov <lav@etersoft.ru>
> ---
>  fs/fcntl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 30f47d0..e9443d9 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -590,17 +590,17 @@ convert_fcntl_cmd(unsigned int cmd)
>   * GETLK was successful and we need to return the data, but it needs to fit in
>   * the compat structure.
>   * l_start shouldn't be too big, unless the original start + end is greater than
> - * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return
> + * off_t_max, in which case the app was asking for trouble, so we return
>   * -EOVERFLOW in that case.  l_len could be too big, in which case we just
>   * truncate it, and only allow the app to see that part of the conflicting lock
>   * that might make sense to it anyway
>   */
> -static int fixup_compat_flock(struct flock *flock)
> +static int fixup_compat_flock(struct flock *flock, loff_t off_t_max)
>  {
> -	if (flock->l_start > COMPAT_OFF_T_MAX)
> +	if (flock->l_start > off_t_max)
>  		return -EOVERFLOW;
> -	if (flock->l_len > COMPAT_OFF_T_MAX)
> -		flock->l_len = COMPAT_OFF_T_MAX;
> +	if (flock->l_len > off_t_max)
> +		flock->l_len = off_t_max;
>  	return 0;
>  }
> 

Wait...

Does this do anything at all in the case where you pass in
COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t
(depending on arch).

Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see
a need to check here.

>  
> @@ -631,7 +631,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
>  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>  		if (err)
>  			break;
> -		err = fixup_compat_flock(&flock);
> +		err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX);
>  		if (err)
>  			return err;
>  		err = put_compat_flock(&flock, compat_ptr(arg));
> @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
>  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>  		if (err)
>  			break;
> -		err = fixup_compat_flock(&flock);
> +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
>  		if (err)
>  			return err;
>  		err = put_compat_flock64(&flock, compat_ptr(arg));

Maybe a simpler fix would be to just remove the fixup_compat_flock call
above?

PS: if you send any more patches, please cc Christoph. He did the
earlier work on cleaning up the compat syscall code, and I'd like him to
be kept in the loop on this as well.

Thanks,
Vitaly Lipatov Nov. 14, 2017, 7:25 p.m. UTC | #3
Jeff Layton писал 14.11.17 22:12:
...
> Wait...
> 
> Does this do anything at all in the case where you pass in
> COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t
> (depending on arch).
> 
> Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see
> a need to check here.
I am not sure, can off_t be bigger than loff_t ?
If not, we have just skip checking against COMPAT_LOFF_T_MAX.

...
>> @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, 
>> unsigned int, cmd,
>>  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>>  		if (err)
>>  			break;
>> -		err = fixup_compat_flock(&flock);
>> +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
>>  		if (err)
>>  			return err;
>>  		err = put_compat_flock64(&flock, compat_ptr(arg));
> 
> Maybe a simpler fix would be to just remove the fixup_compat_flock call
> above?
> 
> PS: if you send any more patches, please cc Christoph. He did the
Ok.
Jeff Layton Nov. 14, 2017, 8:19 p.m. UTC | #4
On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote:
> Jeff Layton писал 14.11.17 22:12:
> ...
> > Wait...
> > 
> > Does this do anything at all in the case where you pass in
> > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t
> > (depending on arch).
> > 
> > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see
> > a need to check here.
> 
> I am not sure, can off_t be bigger than loff_t ?

I don't think so, at least not in any possible situation we care about
here.

> If not, we have just skip checking against COMPAT_LOFF_T_MAX.
> 
> ...
> > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, 
> > > unsigned int, cmd,
> > >  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
> > >  		if (err)
> > >  			break;
> > > -		err = fixup_compat_flock(&flock);
> > > +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
> > >  		if (err)
> > >  			return err;
> > >  		err = put_compat_flock64(&flock, compat_ptr(arg));
> > 
> > Maybe a simpler fix would be to just remove the fixup_compat_flock call
> > above?
> > 

Ok. If you have a test for this, mind testing and sending a patch?

Thanks,
Vitaly Lipatov Nov. 14, 2017, 9:22 p.m. UTC | #5
Jeff Layton писал 14.11.17 23:19:
> On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote:
>> Jeff Layton писал 14.11.17 22:12:
>> ...
>> > Wait...
>> >
>> > Does this do anything at all in the case where you pass in
>> > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t
>> > (depending on arch).
>> >
>> > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see
>> > a need to check here.
>> 
>> I am not sure, can off_t be bigger than loff_t ?
> 
> I don't think so, at least not in any possible situation we care about
> here.
We have this checking for ages:
			if (f.l_start > COMPAT_LOFF_T_MAX)
  				ret = -EOVERFLOW;
http://debian.securedservers.com/kernel/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc5/2.6.15-rc5-mm1/broken-out/fix-overflow-tests-for-compat_sys_fcntl64-locking.patch

> 
>> If not, we have just skip checking against COMPAT_LOFF_T_MAX.
>> 
>> ...
>> > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd,
>> > > unsigned int, cmd,
>> > >  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
>> > >  		if (err)
>> > >  			break;
>> > > -		err = fixup_compat_flock(&flock);
>> > > +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
>> > >  		if (err)
>> > >  			return err;
>> > >  		err = put_compat_flock64(&flock, compat_ptr(arg));
>> >
>> > Maybe a simpler fix would be to just remove the fixup_compat_flock call
>> > above?
>> >
> 
> Ok. If you have a test for this, mind testing and sending a patch?
I think if COMPAT_LOFF_T_MAX is exists, that value can be smaller than 
can fit in off_t.
Checking against COMPAT_LOFF_T_MAX keep old logic works for me last 10 
years.

I have some tests around wine project I worked on. May be later I will 
do additional tests.
Jeff Layton Nov. 15, 2017, 1:16 p.m. UTC | #6
On Wed, 2017-11-15 at 00:22 +0300, Vitaly Lipatov wrote:
> Jeff Layton писал 14.11.17 23:19:
> > On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote:
> > > Jeff Layton писал 14.11.17 22:12:
> > > ...
> > > > Wait...
> > > > 
> > > > Does this do anything at all in the case where you pass in
> > > > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t
> > > > (depending on arch).
> > > > 
> > > > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see
> > > > a need to check here.
> > > 
> > > I am not sure, can off_t be bigger than loff_t ?
> > 
> > I don't think so, at least not in any possible situation we care about
> > here.
> 
> We have this checking for ages:
> 			if (f.l_start > COMPAT_LOFF_T_MAX)
>   				ret = -EOVERFLOW;
> http://debian.securedservers.com/kernel/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc5/2.6.15-rc5-mm1/broken-out/fix-overflow-tests-for-compat_sys_fcntl64-locking.patch
> 

I'm not convinced that those checks ever did anything, tbh.

> > 
> > > If not, we have just skip checking against COMPAT_LOFF_T_MAX.
> > > 
> > > ...
> > > > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd,
> > > > > unsigned int, cmd,
> > > > >  		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
> > > > >  		if (err)
> > > > >  			break;
> > > > > -		err = fixup_compat_flock(&flock);
> > > > > +		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
> > > > >  		if (err)
> > > > >  			return err;
> > > > >  		err = put_compat_flock64(&flock, compat_ptr(arg));
> > > > 
> > > > Maybe a simpler fix would be to just remove the fixup_compat_flock call
> > > > above?
> > > > 
> > 
> > Ok. If you have a test for this, mind testing and sending a patch?
> 
> I think if COMPAT_LOFF_T_MAX is exists, that value can be smaller than 
> can fit in off_t.
> Checking against COMPAT_LOFF_T_MAX keep old logic works for me last 10 
> years.
> 
> I have some tests around wine project I worked on. May be later I will 
> do additional tests.
> 

I am making an assumption here that l_start and l_end can never be
larger than a signed 64-bit value. I don't see how it ever could be,
given that it's defined as a long long, but I suppose we could add some
exotic arch later that does something weird.

Maybe we can just add a BUILD_BUG_ON for that? I'll send along an
alternate patch in a few mins.
diff mbox

Patch

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 30f47d0..e9443d9 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -590,17 +590,17 @@  convert_fcntl_cmd(unsigned int cmd)
  * GETLK was successful and we need to return the data, but it needs to fit in
  * the compat structure.
  * l_start shouldn't be too big, unless the original start + end is greater than
- * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return
+ * off_t_max, in which case the app was asking for trouble, so we return
  * -EOVERFLOW in that case.  l_len could be too big, in which case we just
  * truncate it, and only allow the app to see that part of the conflicting lock
  * that might make sense to it anyway
  */
-static int fixup_compat_flock(struct flock *flock)
+static int fixup_compat_flock(struct flock *flock, loff_t off_t_max)
 {
-	if (flock->l_start > COMPAT_OFF_T_MAX)
+	if (flock->l_start > off_t_max)
 		return -EOVERFLOW;
-	if (flock->l_len > COMPAT_OFF_T_MAX)
-		flock->l_len = COMPAT_OFF_T_MAX;
+	if (flock->l_len > off_t_max)
+		flock->l_len = off_t_max;
 	return 0;
 }
 
@@ -631,7 +631,7 @@  COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
 		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
 		if (err)
 			break;
-		err = fixup_compat_flock(&flock);
+		err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX);
 		if (err)
 			return err;
 		err = put_compat_flock(&flock, compat_ptr(arg));
@@ -644,7 +644,7 @@  COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
 		err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock);
 		if (err)
 			break;
-		err = fixup_compat_flock(&flock);
+		err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX);
 		if (err)
 			return err;
 		err = put_compat_flock64(&flock, compat_ptr(arg));