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

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

[Vitaly Lipatov]

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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 30f47d0..fa17f67 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -604,6 +604,25 @@ static int fixup_compat_flock(struct flock *flock)
 	return 0;
 }
 
+/*
+ * GETLK64 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_LOFF_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_l_flock(struct flock *flock)
+{
+	if (flock->l_start > COMPAT_LOFF_T_MAX)
+		return -EOVERFLOW;
+	if (flock->l_len > COMPAT_LOFF_T_MAX)
+		flock->l_len = COMPAT_LOFF_T_MAX;
+	return 0;
+}
+
 COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
 		       compat_ulong_t, arg)
 {
@@ -644,7 +663,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_l_flock(&flock);
 		if (err)
 			return err;
 		err = put_compat_flock64(&flock, compat_ptr(arg));
-- 
2.10.4

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

[Jeff Layton]

On Tue, 2017-11-14 at 04:30 +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 | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 30f47d0..fa17f67 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -604,6 +604,25 @@ static int fixup_compat_flock(struct flock *flock)
>  	return 0;
>  }
>  
> +/*
> + * GETLK64 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_LOFF_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_l_flock(struct flock *flock)
> +{
> +	if (flock->l_start > COMPAT_LOFF_T_MAX)
> +		return -EOVERFLOW;
> +	if (flock->l_len > COMPAT_LOFF_T_MAX)
> +		flock->l_len = COMPAT_LOFF_T_MAX;
> +	return 0;
> +}
> +

(cc'ing Christoph since he wrote the original patch)

This patch looks correct to me, but could we rename it to
fixup_compat_flock64 to match the other functions here?

Also, I think this should probably go to stable -- any objections?

>  COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd,
>  		       compat_ulong_t, arg)
>  {
> @@ -644,7 +663,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_l_flock(&flock);
>  		if (err)
>  			return err;
>  		err = put_compat_flock64(&flock, compat_ptr(arg));


-- 
Jeff Layton <jlayton@kernel.org>

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

[Vitaly Lipatov]

Jeff Layton писал 14.11.17 14:29:
> (cc'ing Christoph since he wrote the original patch)
> 
> This patch looks correct to me, but could we rename it to
> fixup_compat_flock64 to match the other functions here?
Sure, feel free to make any renames and so on.

-- 
С уважением,
Виталий Липатов,
Etersoft

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

[Vitaly Lipatov]

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
+ * COMPAT_OFF_T_MAX/COMPAT_LOFF_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_OFF_T_MAX);
 		if (err)
 			return err;
 		err = put_compat_flock64(&flock, compat_ptr(arg));
-- 
2.10.4

Re: [PATCH v2] fs/fcntl: restore checking against COMPAT_LOFF_T_MAX for F_GETLK64

[Jeff Layton]

On Tue, 2017-11-14 at 16:47 +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
> + * COMPAT_OFF_T_MAX/COMPAT_LOFF_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_OFF_T_MAX);

I think you want COMPAT_LOFF_T_MAX here? In any case, I'm fine with the
first version, and just renaming the function. I'll plan to push that
one unless you have a reason that we should do it this way.

>  		if (err)
>  			return err;
>  		err = put_compat_flock64(&flock, compat_ptr(arg));

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] fs/fcntl: restore checking against COMPAT_LOFF_T_MAX for F_GETLK64

[Vitaly Lipatov]

Jeff Layton писал 14.11.17 17:06:
...
>>  			break;
>> -		err = fixup_compat_flock(&flock);
>> +		err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX);
> 
> I think you want COMPAT_LOFF_T_MAX here? In any case, I'm fine with the
> first version, and just renaming the function. I'll plan to push that
> one unless you have a reason that we should do it this way.
I would like send v3 with fix the typo you told me. As for me, it is 
more clean than two functions.
Was I wrong with MessageId last time or it is ok to have a new thread 
for every patch version?


-- 
С уважением,
Виталий Липатов,
Etersoft

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

[Vitaly Lipatov]

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;
 }
 
@@ -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

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

[J. Bruce Fields]

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

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

[Jeff Layton]

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,
-- 
Jeff Layton <jlayton@kernel.org>

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

[Vitaly Lipatov]

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.

-- 
С уважением,
Виталий Липатов,
Etersoft

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

[Jeff Layton]

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,
-- 
Jeff Layton <jlayton@kernel.org>

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

[Vitaly Lipatov]

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.

-- 
С уважением,
Виталий Липатов,
Etersoft

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

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>