Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>,
        Prashanth K <quic_prashk@quicinc.com>
CC:     "Gustavo A . R . Silva" <gustavoars@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        John Keeping <john@metanate.com>,
        Linyu Yuan <quic_linyyuan@quicinc.com>,
        Pratham Pratap <quic_ppratap@quicinc.com>,
        Vincent Pelletier <plr.vincent@gmail.com>,
        Dan Carpenter <error27@gmail.com>,
        Udipto Goswami <quic_ugoswami@quicinc.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "# 5 . 15" <stable@vger.kernel.org>
Subject: RE: usb: f_fs: Fix CFI failure in ki_complete
Thread-Topic: usb: f_fs: Fix CFI failure in ki_complete
Thread-Index: AQHZDjGpwikydY+vnkC/L44cbfn06q5tpHEA
Date:   Wed, 14 Dec 2022 17:35:01 +0000
Message-ID: <abe47a47aa5d49878c58fc1199be18ea@AcuMS.aculab.com>
References: <1670851464-8106-1-git-send-email-quic_prashk@quicinc.com>
 <Y5cuCMhFIaKraUyi@kroah.com>
In-Reply-To: <Y5cuCMhFIaKraUyi@kroah.com>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

From: Greg Kroah-Hartman
> Sent: 12 December 2022 13:35
> 
> On Mon, Dec 12, 2022 at 06:54:24PM +0530, Prashanth K wrote:
> > Function pointer ki_complete() expects 'long' as its second
> > argument, but we pass integer from ffs_user_copy_worker. This
> > might cause a CFI failure, as ki_complete is an indirect call
> > with mismatched prototype. Fix this by typecasting the second
> > argument to long.
> 
> "might"?  Does it or not?  If it does, why hasn't this been reported
> before?

Does the cast even help at all.

...
> > -	io_data->kiocb->ki_complete(io_data->kiocb, ret);
> > +	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);
...

If definition of the parameter in the structure member ki_complete()
definition is 'long' then the compiler has to promote 'ret' to long
anyway. CFI has nothing to do with it.

OTOH if you've used a cast to assign a function with a
different prototype to ki_complete then 'all bets are off'
and you get all the run time errors you deserve.
CFI just converts some of them to compile time errors.

For instance if you assign xx_complete(long) to (*ki_complete)(int)
then it is very likely that xx_complete() will an argument
with some of the high bits set.
But adding a cast to the call - ki_complete((long)int_var)
will make absolutely no difference.
The compiler wont zero/sign extend int_var to 64bits for you,
that will just get optimised away and the high bits will
be unchanged.

You're description seems to be the other way around (which might
be safe, but CFI probably still barfs).
But you need to fix the indirect calls so the function types
match.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)