[next] cifsd: Fix a less than zero comparison with the unsigned int nbytes

Commit Message

Colin King Oct. 7, 2021, 11:47 a.m. UTC

From: Colin Ian King <[e-mail]>

Currently the check for nbytes < 0 is always false because nbytes
is an unsigned int and can never be less than zero.  Fix this by
using ret for the assignment and comparison and assigning nbytes
to ret later if the check is successful. The fix also passes the
error return in ret to the error handling path that caters for
various values of ret.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Signed-off-by: Colin Ian King <[e-mail]>
---
 fs/ksmbd/smb2pdu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Namjae Jeon Oct. 7, 2021, 12:37 p.m. UTC | #1

2021-10-07 20:47 GMT+09:00, Colin King <[e-mail]>:
> From: Colin Ian King <[e-mail]>
>
> Currently the check for nbytes < 0 is always false because nbytes
> is an unsigned int and can never be less than zero.  Fix this by
> using ret for the assignment and comparison and assigning nbytes
> to ret later if the check is successful. The fix also passes the
> error return in ret to the error handling path that caters for
> various values of ret.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
I think that this alarm is caused by 	b66732021c64 (ksmbd: add
validation in smb2_ioctl).
Fixes tag may be not needed. Because b66732021c64 patch is not applied
to Linus' tree yet ?
> Signed-off-by: Colin Ian King <[e-mail]>
Acked-by: Namjae Jeon <[e-mail]>

Thanks!

Namjae Jeon Oct. 7, 2021, 12:51 p.m. UTC | #2

2021-10-07 21:37 GMT+09:00, Namjae Jeon <[e-mail]>:
> 2021-10-07 20:47 GMT+09:00, Colin King <[e-mail]>:
>> From: Colin Ian King <[e-mail]>
>>
>> Currently the check for nbytes < 0 is always false because nbytes
>> is an unsigned int and can never be less than zero.  Fix this by
>> using ret for the assignment and comparison and assigning nbytes
>> to ret later if the check is successful. The fix also passes the
>> error return in ret to the error handling path that caters for
>> various values of ret.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> I think that this alarm is caused by 	b66732021c64 (ksmbd: add
> validation in smb2_ioctl).
> Fixes tag may be not needed. Because b66732021c64 patch is not applied
> to Linus' tree yet ?
>> Signed-off-by: Colin Ian King <[e-mail]>
> Acked-by: Namjae Jeon <[e-mail]>

I found one issue in this patch.
if ret is -EINVAL, Status is changed to STATUS_INVALID_PARAMETER from
STATUS_BUFFER_TOO_SMALL.

static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
                                        struct smb2_ioctl_rsp *rsp,
                                        unsigned int out_buf_len)
...
        if (!nbytes) {
                rsp->hdr.Status = STATUS_BUFFER_TOO_SMALL;
                return -EINVAL;
        }

>
> Thanks!
>

Dan Carpenter Oct. 7, 2021, 1:35 p.m. UTC | #3

On Thu, Oct 07, 2021 at 09:37:04PM +0900, Namjae Jeon wrote:
> 2021-10-07 20:47 GMT+09:00, Colin King <[e-mail]>:
> >
> > Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> I think that this alarm is caused by 	b66732021c64 (ksmbd: add
> validation in smb2_ioctl).
> Fixes tag may be not needed. Because b66732021c64 patch is not applied
> to Linus' tree yet ?

If you are going to modify the commit to include this fix then that's
fine.  Otherise if you are going to apply this commit then the Fixes
tag is still required.

The fixes tag saves time for backporters because they can automatically
rule out that this patch needs to be backported.  Or if they backport
commit b66732021c64 then they know they have to backport the fix as
well.

Also the Fixes tag is used for other purposes besides backporting.
It helps review.  It's also an interesting metric to measure how long
between the bug is introduced and the fix is applied.

regards,
dan carpenter

Namjae Jeon Oct. 7, 2021, 2:31 p.m. UTC | #4

2021-10-07 22:35 GMT+09:00, Dan Carpenter <[e-mail]>:
> On Thu, Oct 07, 2021 at 09:37:04PM +0900, Namjae Jeon wrote:
>> 2021-10-07 20:47 GMT+09:00, Colin King <[e-mail]>:
>> >
>> > Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
>> I think that this alarm is caused by 	b66732021c64 (ksmbd: add
>> validation in smb2_ioctl).
>> Fixes tag may be not needed. Because b66732021c64 patch is not applied
>> to Linus' tree yet ?
>
> If you are going to modify the commit to include this fix then that's
> fine.  Otherise if you are going to apply this commit then the Fixes
> tag is still required.
>
> The fixes tag saves time for backporters because they can automatically
> rule out that this patch needs to be backported.  Or if they backport
> commit b66732021c64 then they know they have to backport the fix as
> well.
>
> Also the Fixes tag is used for other purposes besides backporting.
> It helps review.  It's also an interesting metric to measure how long
> between the bug is introduced and the fix is applied.
Okay, Thanks for your detailed explanation:)
>
> regards,
> dan carpenter
>
>

Patch

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 8ceac0ebdbea..9be82d08b722 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7537,9 +7537,10 @@  int smb2_ioctl(struct ksmbd_work *work)
 		rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
 		break;
 	case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
-		nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
-		if (nbytes < 0)
+		ret = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
+		if (ret < 0)
 			goto out;
+		nbytes = ret;
 		break;
 	case FSCTL_REQUEST_RESUME_KEY:
 		if (out_buf_len < sizeof(struct resume_key_ioctl_rsp)) {