| Message ID | 20230118170657.17585-1-ematsumiya@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: do not include page data when checking signature | expand |
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>
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>
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>
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
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
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.
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",
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(-)