diff mbox series

usb: f_fs: Fix CFI failure in ki_complete

Message ID 1670851464-8106-1-git-send-email-quic_prashk@quicinc.com
State New
Headers
Series usb: f_fs: Fix CFI failure in ki_complete |

Commit Message

Prashanth K Dec. 12, 2022, 1:24 p.m. UTC 
  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.

Cc: <stable@vger.kernel.org> # 5.15
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>

---
 drivers/usb/gadget/function/f_fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg KH Dec. 12, 2022, 1:35 p.m. UTC | #1 
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?

> Cc: <stable@vger.kernel.org> # 5.15

CFI first showed up in 6.1, not 5.15, right?

> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> 
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73dc10a7..9c26561 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -835,7 +835,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  		kthread_unuse_mm(io_data->mm);
>  	}
>  
> -	io_data->kiocb->ki_complete(io_data->kiocb, ret);
> +	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);

Why just fix up this one instance?  What about ep_user_copy_worker()?
And what about all other calls to ki_complete that are not using a
(long) cast?

This feels wrong, what exactly is the reported error and how come other
kernel calls to this function pointer have not had a problem with CFI?
ceph_aio_complete() would be another example, right?

thanks,

greg k-h
John Keeping Dec. 12, 2022, 1:37 p.m. UTC | #2 
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.
> 
> Cc: <stable@vger.kernel.org> # 5.15
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> 
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73dc10a7..9c26561 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -835,7 +835,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  		kthread_unuse_mm(io_data->mm);
>  	}
>  
> -	io_data->kiocb->ki_complete(io_data->kiocb, ret);
> +	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);

I don't understand the problem here, ki_complete() is declared with a
long parameter, so won't integer promotion kick in so that the function
is called with a long?  Why is the explicit cast necessary?

>  
>  	if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd)
>  		eventfd_signal(io_data->ffs->ffs_eventfd, 1);
> -- 
> 2.7.4
>
Dan Carpenter Dec. 12, 2022, 1:49 p.m. UTC | #3 
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.
> 
> Cc: <stable@vger.kernel.org> # 5.15
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> 
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73dc10a7..9c26561 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -835,7 +835,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  		kthread_unuse_mm(io_data->mm);
>  	}
>  
> -	io_data->kiocb->ki_complete(io_data->kiocb, ret);
> +	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);

I don't understand this commit at all.  CFI is Control Flow Integrity
or Common Flash Interface depending on which subsystem we're talking
about.

I really think that Clang needs to be fixed if this really causes an
issue for Clang.  How on earth are we going to know where to add all
the casts?

The commit message says "this might cause a CFI" failure.  Either it
does or it doesn't.  Please someone test this so we can know what's
going on.

Why is it backported to 5.15?  I thought CFI was not going to backported
that far and I has seen people complaining about CFI backports.

regards,
dan carpenter
Prashanth K Dec. 14, 2022, 1:08 p.m. UTC | #4 
On 12-12-22 07:05 pm, Greg Kroah-Hartman wrote:
> 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?
Sorry for the confusion in commit text, We caught a CFI (Control Flow 
Integrity) failure internally on 5.15, hence pushed this patch. But 
later I came to know that CFI was implemented on 5.4 kernel for Android. 
Will push the same on ACK and share the related details there.

Thanks.
> 
>> Cc: <stable@vger.kernel.org> # 5.15
> 
> CFI first showed up in 6.1, not 5.15, right?
> 
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>>
>> ---
>>   drivers/usb/gadget/function/f_fs.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 73dc10a7..9c26561 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -835,7 +835,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
>>   		kthread_unuse_mm(io_data->mm);
>>   	}
>>   
>> -	io_data->kiocb->ki_complete(io_data->kiocb, ret);
>> +	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);
> 
> Why just fix up this one instance?  What about ep_user_copy_worker()?
> And what about all other calls to ki_complete that are not using a
> (long) cast?
> 
> This feels wrong, what exactly is the reported error and how come other
> kernel calls to this function pointer have not had a problem with CFI?
> ceph_aio_complete() would be another example, right?
> 
> thanks,
> 
> greg k-h
Greg KH Dec. 14, 2022, 2:56 p.m. UTC | #5 
On Wed, Dec 14, 2022 at 06:38:17PM +0530, Prashanth K wrote:
> 
> 
> On 12-12-22 07:05 pm, Greg Kroah-Hartman wrote:
> > 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?
> Sorry for the confusion in commit text, We caught a CFI (Control Flow
> Integrity) failure internally on 5.15, hence pushed this patch. But later I
> came to know that CFI was implemented on 5.4 kernel for Android. Will push
> the same on ACK and share the related details there.

I will have the same questions there, namely, "why just this one
instance and why is it trigging anything"?

So please, work this out here, in public, don't bury stuff in random
vendor kernel trees.  That's not the way to solve anything properly, you
know this :)

thanks,

greg k-h
David Laight Dec. 14, 2022, 5:35 p.m. UTC | #6 
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)
Prashanth K Dec. 22, 2022, 12:51 p.m. UTC | #7 
On 14-12-22 11:05 pm, David Laight wrote:
> 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.
Actually I also have these same questions
- why we haven't seen any instances other than this one?
- why its not seen on other indirect function calls?

Here is the the call stack of the failure that we got.

[  323.288681][    T7] Kernel panic - not syncing: CFI failure (target: 
0xffffffe5fc811f98)
[  323.288710][    T7] CPU: 6 PID: 7 Comm: kworker/u16:0 Tainted: G S 
    W  OE     5.15.41-android13-8-g5ffc5644bd20 #1
[  323.288730][    T7] Workqueue: adb ffs_user_copy_worker.cfi_jt
[  323.288752][    T7] Call trace:
[  323.288755][    T7]  dump_backtrace.cfi_jt+0x0/0x8
[  323.288772][    T7]  dump_stack_lvl+0x80/0xb8
[  323.288785][    T7]  panic+0x180/0x444
[  323.288797][    T7]  find_check_fn+0x0/0x218
[  323.288810][    T7]  ffs_user_copy_worker+0x1dc/0x204
[  323.288822][    T7]  kretprobe_trampoline.cfi_jt+0x0/0x8
[  323.288837][    T7]  worker_thread+0x3ec/0x920
[  323.288850][    T7]  kthread+0x168/0x1dc
[  323.288859][    T7]  ret_from_fork+0x10/0x20
[  323.288866][    T7] SMP: stopping secondary CPUs

And from address to line translation, we got know the issue is from
ffs_user_copy_worker+0x1dc/0x204
		||
io_data->kiocb->ki_complete(io_data->kiocb, ret);

And "find_check_fn" was getting invoked from ki_complete. Only thing 
that I found suspicious about ki_complete() is its argument types. 
That's why I pushed this patch here, so that we can discuss this out here.

Thanks in advance

> 
> ...
>>> -	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.
So does that mean, we need to add casts in al indirect calls to match 
the function signature?
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Dan Carpenter Dec. 23, 2022, 9:04 a.m. UTC | #8 
On Thu, Dec 22, 2022 at 06:21:03PM +0530, Prashanth K wrote:
> 
> 
> On 14-12-22 11:05 pm, David Laight wrote:
> > 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.
> Actually I also have these same questions
> - why we haven't seen any instances other than this one?
> - why its not seen on other indirect function calls?
> 
> Here is the the call stack of the failure that we got.
> 
> [  323.288681][    T7] Kernel panic - not syncing: CFI failure (target:
> 0xffffffe5fc811f98)
> [  323.288710][    T7] CPU: 6 PID: 7 Comm: kworker/u16:0 Tainted: G S    W
> OE     5.15.41-android13-8-g5ffc5644bd20 #1
> [  323.288730][    T7] Workqueue: adb ffs_user_copy_worker.cfi_jt
> [  323.288752][    T7] Call trace:
> [  323.288755][    T7]  dump_backtrace.cfi_jt+0x0/0x8
> [  323.288772][    T7]  dump_stack_lvl+0x80/0xb8
> [  323.288785][    T7]  panic+0x180/0x444
> [  323.288797][    T7]  find_check_fn+0x0/0x218
> [  323.288810][    T7]  ffs_user_copy_worker+0x1dc/0x204
> [  323.288822][    T7]  kretprobe_trampoline.cfi_jt+0x0/0x8
> [  323.288837][    T7]  worker_thread+0x3ec/0x920
> [  323.288850][    T7]  kthread+0x168/0x1dc
> [  323.288859][    T7]  ret_from_fork+0x10/0x20
> [  323.288866][    T7] SMP: stopping secondary CPUs
> 
> And from address to line translation, we got know the issue is from
> ffs_user_copy_worker+0x1dc/0x204
> 		||
> io_data->kiocb->ki_complete(io_data->kiocb, ret);
> 
> And "find_check_fn" was getting invoked from ki_complete. Only thing that I
> found suspicious about ki_complete() is its argument types. That's why I
> pushed this patch here, so that we can discuss this out here.

I think the problem is more likely whatever ->ki_complete() points to
but I have no idea what that is on your system.  You're using an Android
kernel so it could be something out of tree as well...

regards,
dan carpenter
Alexander Lobakin Dec. 23, 2022, 2:41 p.m. UTC | #9 
From: Dan Carpenter <error27@gmail.com>
Date: Fri, 23 Dec 2022 12:04:18 +0300

> On Thu, Dec 22, 2022 at 06:21:03PM +0530, Prashanth K wrote:
> > 
> > 
> > On 14-12-22 11:05 pm, David Laight wrote:
> > > 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.
> > Actually I also have these same questions
> > - why we haven't seen any instances other than this one?
> > - why its not seen on other indirect function calls?
> > 
> > Here is the the call stack of the failure that we got.
> > 
> > [  323.288681][    T7] Kernel panic - not syncing: CFI failure (target:
> > 0xffffffe5fc811f98)
> > [  323.288710][    T7] CPU: 6 PID: 7 Comm: kworker/u16:0 Tainted: G S    W
> > OE     5.15.41-android13-8-g5ffc5644bd20 #1
> > [  323.288730][    T7] Workqueue: adb ffs_user_copy_worker.cfi_jt
> > [  323.288752][    T7] Call trace:
> > [  323.288755][    T7]  dump_backtrace.cfi_jt+0x0/0x8
> > [  323.288772][    T7]  dump_stack_lvl+0x80/0xb8
> > [  323.288785][    T7]  panic+0x180/0x444
> > [  323.288797][    T7]  find_check_fn+0x0/0x218
> > [  323.288810][    T7]  ffs_user_copy_worker+0x1dc/0x204
> > [  323.288822][    T7]  kretprobe_trampoline.cfi_jt+0x0/0x8
> > [  323.288837][    T7]  worker_thread+0x3ec/0x920
> > [  323.288850][    T7]  kthread+0x168/0x1dc
> > [  323.288859][    T7]  ret_from_fork+0x10/0x20
> > [  323.288866][    T7] SMP: stopping secondary CPUs
> > 
> > And from address to line translation, we got know the issue is from
> > ffs_user_copy_worker+0x1dc/0x204
> > 		||
> > io_data->kiocb->ki_complete(io_data->kiocb, ret);
> > 
> > And "find_check_fn" was getting invoked from ki_complete. Only thing that I
> > found suspicious about ki_complete() is its argument types. That's why I
> > pushed this patch here, so that we can discuss this out here.
> 
> I think the problem is more likely whatever ->ki_complete() points to
> but I have no idea what that is on your system.  You're using an Android
> kernel so it could be something out of tree as well...

Correct, CFI would *never* trigger a failure due to passing int as
long. It triggers only on prototype-implementation mismatches. The
author should go and check carefully whether there are any places
where some implementation differs and then ::ki_complete() gets
passed with a function typecast. Also, there can be places where a
proto has an argument as enum and an implementation has it as int.
Compilers don't warn on such mismatches, CFI does. The latest LLVM
Git snapshot with `-Wcast-function-type-strict` enabled could help
hunt such.

> 
> regards,
> dan carpenter

Thanks,
Olek
Greg KH Dec. 23, 2022, 2:51 p.m. UTC | #10 
On Thu, Dec 22, 2022 at 06:21:03PM +0530, Prashanth K wrote:
> 
> 
> On 14-12-22 11:05 pm, David Laight wrote:
> > 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.
> Actually I also have these same questions
> - why we haven't seen any instances other than this one?
> - why its not seen on other indirect function calls?

Great, please work on figuring these out before you resubmit this again
as obviously we can't take this change without knowing the answers here.

good luck!

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73dc10a7..9c26561 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -835,7 +835,7 @@  static void ffs_user_copy_worker(struct work_struct *work)
 		kthread_unuse_mm(io_data->mm);
 	}
 
-	io_data->kiocb->ki_complete(io_data->kiocb, ret);
+	io_data->kiocb->ki_complete(io_data->kiocb, (long)ret);
 
 	if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd)
 		eventfd_signal(io_data->ffs->ffs_eventfd, 1);