| Message ID | 20230519013710.34954-1-suhui@nfschina.com |
|---|---|
| State | New |
| Headers | show |
| Series | drivers/fsi/scom: Return -EFAULT if copy fails | expand |
On Fri, May 19, 2023 at 09:37:10AM +0800, Su Hui wrote: > The copy_to/from_user() functions return the number of bytes remaining > to be copied, but we want to return -EFAULT to the user. > Why ? EFAULT means that a bad address was provided, and it is not immediately obvious why that would be the case. Guenter > Fixes: 680ca6dcf5c2 ("drivers/fsi: Add SCOM FSI client device driver") > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > drivers/fsi/fsi-scom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c > index bcb756dc9866..caaf7738eb98 100644 > --- a/drivers/fsi/fsi-scom.c > +++ b/drivers/fsi/fsi-scom.c > @@ -335,7 +335,7 @@ static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, > if (rc) > dev_dbg(dev, "copy to user failed:%d\n", rc); > > - return rc ? rc : len; > + return rc ? -EFAULT : len; > } > > static ssize_t scom_write(struct file *filep, const char __user *buf, > -- > 2.30.2 >
On 2023/5/23 06:30, Guenter Roeck wrote: > On Fri, May 19, 2023 at 09:37:10AM +0800, Su Hui wrote: >> The copy_to/from_user() functions return the number of bytes remaining >> to be copied, but we want to return -EFAULT to the user. >> > Why ? EFAULT means that a bad address was provided, and it is not > immediately obvious why that would be the case. When copy_to/from_user() functions failed, the error code is -EFAULT in most case. git grep -A1 "copy_from_user" | grep EFAULT | wc -l 1985 git grep -A1 "copy_to_user" | grep EFAULT | wc -l 1871 I think return -EFAULT is also right in this case. Su Hui > Guenter > >> Fixes: 680ca6dcf5c2 ("drivers/fsi: Add SCOM FSI client device driver") >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> drivers/fsi/fsi-scom.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c >> index bcb756dc9866..caaf7738eb98 100644 >> --- a/drivers/fsi/fsi-scom.c >> +++ b/drivers/fsi/fsi-scom.c >> @@ -335,7 +335,7 @@ static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, >> if (rc) >> dev_dbg(dev, "copy to user failed:%d\n", rc); >> >> - return rc ? rc : len; >> + return rc ? -EFAULT : len; >> } >> >> static ssize_t scom_write(struct file *filep, const char __user *buf, >> -- >> 2.30.2 >>
On Mon, May 22, 2023 at 03:30:06PM -0700, Guenter Roeck wrote: > On Fri, May 19, 2023 at 09:37:10AM +0800, Su Hui wrote: > > The copy_to/from_user() functions return the number of bytes remaining > > to be copied, but we want to return -EFAULT to the user. > > > Why ? EFAULT means that a bad address was provided, and it is not > immediately obvious why that would be the case. > Right now the function is returning success so that's definitely wrong. The copy_to/from_user() function will only fail if a bad address is provided so -EFAULT is correct. There is a different Smatch warning for when people return a different error code such as -EINVAL. drivers/fsi/fsi-scom.c:337 scom_read() warn: return -EFAULT instead of '-EINVAL' The return affects the user and -EFAULT but that level of pendantry feel like possibly too much? There aren't many instances of this warning. regards, dan carpenter
diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c index bcb756dc9866..caaf7738eb98 100644 --- a/drivers/fsi/fsi-scom.c +++ b/drivers/fsi/fsi-scom.c @@ -335,7 +335,7 @@ static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, if (rc) dev_dbg(dev, "copy to user failed:%d\n", rc); - return rc ? rc : len; + return rc ? -EFAULT : len; } static ssize_t scom_write(struct file *filep, const char __user *buf,
The copy_to/from_user() functions return the number of bytes remaining to be copied, but we want to return -EFAULT to the user. Fixes: 680ca6dcf5c2 ("drivers/fsi: Add SCOM FSI client device driver") Signed-off-by: Su Hui <suhui@nfschina.com> --- drivers/fsi/fsi-scom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)