[PATCH] cifs: do not include page data when checking signature

[PATCH] cifs: do not include page data when checking signature

[Enzo Matsumiya]

On async reads, page data is allocated before sending.  When the
response is received but it has no data to fill (e.g.
STATUS_END_OF_FILE), __calc_signature() will still include the pages in
its computation, leading to an invalid signature check.

This patch fixes this by not setting the async read smb_rqst page data
(zeroed by default) if its got_bytes is 0.

This can be reproduced/verified with xfstests generic/465.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/smb2pdu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 4b71f4a92f76..2c9ffa921e6f 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4163,12 +4163,15 @@ smb2_readv_callback(struct mid_q_entry *mid)
 				(struct smb2_hdr *)rdata->iov[0].iov_base;
 	struct cifs_credits credits = { .value = 0, .instance = 0 };
 	struct smb_rqst rqst = { .rq_iov = &rdata->iov[1],
-				 .rq_nvec = 1,
-				 .rq_pages = rdata->pages,
-				 .rq_offset = rdata->page_offset,
-				 .rq_npages = rdata->nr_pages,
-				 .rq_pagesz = rdata->pagesz,
-				 .rq_tailsz = rdata->tailsz };
+				 .rq_nvec = 1, };
+
+	if (rdata->got_bytes) {
+		rqst.rq_pages = rdata->pages;
+		rqst.rq_offset = rdata->page_offset;
+		rqst.rq_npages = rdata->nr_pages;
+		rqst.rq_pagesz = rdata->pagesz;
+		rqst.rq_tailsz = rdata->tailsz;
+	}
 
 	WARN_ONCE(rdata->server != mid->server,
 		  "rdata server %p != mid server %p",
-- 
2.35.3

Re: [PATCH] cifs: do not include page data when checking signature

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On async reads, page data is allocated before sending.  When the
> response is received but it has no data to fill (e.g.
> STATUS_END_OF_FILE), __calc_signature() will still include the pages in
> its computation, leading to an invalid signature check.
>
> This patch fixes this by not setting the async read smb_rqst page data
> (zeroed by default) if its got_bytes is 0.
>
> This can be reproduced/verified with xfstests generic/465.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
>  fs/cifs/smb2pdu.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

Re: [PATCH] cifs: do not include page data when checking signature

[Steve French]

merged into cifs-2.6.git for-next pending additional testing

On Wed, Jan 18, 2023 at 2:07 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Enzo Matsumiya <ematsumiya@suse.de> writes:
>
> > On async reads, page data is allocated before sending.  When the
> > response is received but it has no data to fill (e.g.
> > STATUS_END_OF_FILE), __calc_signature() will still include the pages in
> > its computation, leading to an invalid signature check.
> >
> > This patch fixes this by not setting the async read smb_rqst page data
> > (zeroed by default) if its got_bytes is 0.
> >
> > This can be reproduced/verified with xfstests generic/465.
> >
> > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> > ---
> >  fs/cifs/smb2pdu.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>



-- 
Thanks,

Steve

Re: [PATCH] cifs: do not include page data when checking signature

[Steve French]

I wasn't able to reproduce this with generic/465 - at least not
running to current Samba.  Any thoughts on how to reproduce the
original problem?

On Wed, Jan 18, 2023 at 2:07 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Enzo Matsumiya <ematsumiya@suse.de> writes:
>
> > On async reads, page data is allocated before sending.  When the
> > response is received but it has no data to fill (e.g.
> > STATUS_END_OF_FILE), __calc_signature() will still include the pages in
> > its computation, leading to an invalid signature check.
> >
> > This patch fixes this by not setting the async read smb_rqst page data
> > (zeroed by default) if its got_bytes is 0.
> >
> > This can be reproduced/verified with xfstests generic/465.
> >
> > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> > ---
> >  fs/cifs/smb2pdu.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>



-- 
Thanks,

Steve

Re: [PATCH] cifs: do not include page data when checking signature

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> I wasn't able to reproduce this with generic/465 - at least not
> running to current Samba.  Any thoughts on how to reproduce the
> original problem?

The test actually doesn't fail.  You can, however, observe the signature
verification failures on dmesg while running it, e.g.

        CIFS: VFS: \\ada.test\test SMB signature verification returned error = -13

Re: [PATCH] cifs: do not include page data when checking signature

[ronnie sahlberg]

We should update the buildbot so we clear dmesg before every test and
then check for and fail any test that results in something unexpected.

On Thu, 19 Jan 2023 at 08:18, Steve French <smfrench@gmail.com> wrote:
>
> Aah yes. I see it now
>
> On Wed, Jan 18, 2023, 16:17 Paulo Alcantara <pc@cjr.nz> wrote:
>>
>> Steve French <smfrench@gmail.com> writes:
>>
>> > I wasn't able to reproduce this with generic/465 - at least not
>> > running to current Samba.  Any thoughts on how to reproduce the
>> > original problem?
>>
>> The test actually doesn't fail.  You can, however, observe the signature
>> verification failures on dmesg while running it, e.g.
>>
>>         CIFS: VFS: \\ada.test\test SMB signature verification returned error = -13

Re: [PATCH] cifs: do not include page data when checking signature

[Paulo Alcantara]

ronnie sahlberg <ronniesahlberg@gmail.com> writes:

> We should update the buildbot so we clear dmesg before every test and
> then check for and fail any test that results in something unexpected.

Yes, agreed.

Our internal buildbot does this after running a single test

        bad_dmesg_rx = re.compile(r'Call trace:|blocked for more than.*seconds|BUG: sleeping function called from invalid context|^WARNING|possible recursive locking detected')
        if bad_dmesg_rx.search(dmesg):
            ...

We should probably have 'CIFS VFS:' in that regex as well.