linux-kernel - Re: [PATCH v3] fs/fcntl: restore checking against COMPAT_LOFF_T_MAX for F

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1510686778.4757.11.camel@kernel.org>
Date:   Tue, 14 Nov 2017 14:12:58 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Vitaly Lipatov <lav@...rsoft.ru>,
        wine-patches <wine-patches@...ehq.org>
Cc:     "J. Bruce Fields" <bfields@...ldses.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v3] fs/fcntl: restore checking against COMPAT_LOFF_T_MAX
 for F_GETLK64

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@...rsoft.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@...nel.org>