| Message ID | 20220918235442.2981-1-ematsumiya@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: check if mid was deleted in async read callback | expand |
LGTM reviewed-by me. Good catch about the race! On Mon, 19 Sept 2022 at 09:54, Enzo Matsumiya <ematsumiya@suse.de> wrote: > > There's a race when cifs_readv_receive() might dequeue the mid, > and mid->callback(), called from demultiplex thread, will try to > access it to verify the signature before the mid is actually > released/deleted. > > Currently the signature verification fails, but the verification > shouldn't have happened at all because the mid was deleted because > of an error, and hence not really supposed to be passed to > ->callback(). There are no further errors because the mid is > effectivelly gone by the end of the callback. > > This patch checks if the mid doesn't have the MID_DELETED flag set (by > dequeue_mid()) right before trying to verify the signature. According to > my tests, trying to check it earlier, e.g. after the ->receive() call in > cifs_demultiplex_thread, will fail most of the time as dequeue_mid() > might not have been called yet. > > This behaviour can be seen in xfstests generic/465, for example, where > mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be > discarded, but instead have their signature computed, but mismatched. > > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> > --- > fs/cifs/cifssmb.c | 2 +- > fs/cifs/smb2pdu.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index addf3fc62aef..116f6afe33c6 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -1308,7 +1308,7 @@ cifs_readv_callback(struct mid_q_entry *mid) > switch (mid->mid_state) { > case MID_RESPONSE_RECEIVED: > /* result already set, check signature */ > - if (server->sign) { > + if (server->sign && !(mid->mid_flags & MID_DELETED)) { > int rc = 0; > > rc = cifs_verify_signature(&rqst, server, > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 5da0b596c8a0..394996c4f729 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -4136,7 +4136,7 @@ smb2_readv_callback(struct mid_q_entry *mid) > credits.value = le16_to_cpu(shdr->CreditRequest); > credits.instance = server->reconnect_instance; > /* result already set, check signature */ > - if (server->sign && !mid->decrypted) { > + if (server->sign && !mid->decrypted && !(mid->mid_flags & MID_DELETED)) { > int rc; > > rc = smb2_verify_signature(&rqst, server); > -- > 2.35.3 >
Enzo Matsumiya <ematsumiya@suse.de> writes: > There's a race when cifs_readv_receive() might dequeue the mid, > and mid->callback(), called from demultiplex thread, will try to > access it to verify the signature before the mid is actually > released/deleted. > > Currently the signature verification fails, but the verification > shouldn't have happened at all because the mid was deleted because > of an error, and hence not really supposed to be passed to > ->callback(). There are no further errors because the mid is > effectivelly gone by the end of the callback. > > This patch checks if the mid doesn't have the MID_DELETED flag set (by > dequeue_mid()) right before trying to verify the signature. According to > my tests, trying to check it earlier, e.g. after the ->receive() call in > cifs_demultiplex_thread, will fail most of the time as dequeue_mid() > might not have been called yet. > > This behaviour can be seen in xfstests generic/465, for example, where > mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be > discarded, but instead have their signature computed, but mismatched. > > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> > --- > fs/cifs/cifssmb.c | 2 +- > fs/cifs/smb2pdu.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Good catch! Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
merged into cifs-2.6.git for-next On Mon, Sep 19, 2022 at 9:43 AM Paulo Alcantara <pc@cjr.nz> wrote: > > Enzo Matsumiya <ematsumiya@suse.de> writes: > > > There's a race when cifs_readv_receive() might dequeue the mid, > > and mid->callback(), called from demultiplex thread, will try to > > access it to verify the signature before the mid is actually > > released/deleted. > > > > Currently the signature verification fails, but the verification > > shouldn't have happened at all because the mid was deleted because > > of an error, and hence not really supposed to be passed to > > ->callback(). There are no further errors because the mid is > > effectivelly gone by the end of the callback. > > > > This patch checks if the mid doesn't have the MID_DELETED flag set (by > > dequeue_mid()) right before trying to verify the signature. According to > > my tests, trying to check it earlier, e.g. after the ->receive() call in > > cifs_demultiplex_thread, will fail most of the time as dequeue_mid() > > might not have been called yet. > > > > This behaviour can be seen in xfstests generic/465, for example, where > > mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be > > discarded, but instead have their signature computed, but mismatched. > > > > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> > > --- > > fs/cifs/cifssmb.c | 2 +- > > fs/cifs/smb2pdu.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > Good catch! > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
On 09/19, Steve French wrote: >merged into cifs-2.6.git for-next Steve, please drop this one, I was wrong :( I'll send a new patch later today. > >On Mon, Sep 19, 2022 at 9:43 AM Paulo Alcantara <pc@cjr.nz> wrote: >> >> Enzo Matsumiya <ematsumiya@suse.de> writes: >> >> > There's a race when cifs_readv_receive() might dequeue the mid, >> > and mid->callback(), called from demultiplex thread, will try to >> > access it to verify the signature before the mid is actually >> > released/deleted. >> > >> > Currently the signature verification fails, but the verification >> > shouldn't have happened at all because the mid was deleted because >> > of an error, and hence not really supposed to be passed to >> > ->callback(). There are no further errors because the mid is >> > effectivelly gone by the end of the callback. >> > >> > This patch checks if the mid doesn't have the MID_DELETED flag set (by >> > dequeue_mid()) right before trying to verify the signature. According to >> > my tests, trying to check it earlier, e.g. after the ->receive() call in >> > cifs_demultiplex_thread, will fail most of the time as dequeue_mid() >> > might not have been called yet. >> > >> > This behaviour can be seen in xfstests generic/465, for example, where >> > mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be >> > discarded, but instead have their signature computed, but mismatched. >> > >> > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> >> > --- >> > fs/cifs/cifssmb.c | 2 +- >> > fs/cifs/smb2pdu.c | 2 +- >> > 2 files changed, 2 insertions(+), 2 deletions(-) >> >> Good catch! >> >> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > > >-- >Thanks, > >Steve
I guess I caught up out-of-order, replying to the other thread. Catching the race is good, but "dequeuing the MID" has nothing to do with signing and should not be listed as justification. If the message is being processed, e.g. returning the status field, then the payload MUST be validated per the processing in 3.2.5.1.3. This validation requires only a valid session, and the message itself. Apparently the code is storing the decryption status in the local mid structure. That's the root-cause bug here. The signing validation must not be skipped otherwise! Poking holes in security is never a good approach. Can the decryption boolean be stored someplace else? Tom. On 9/20/2022 12:15 AM, Steve French wrote: > merged into cifs-2.6.git for-next > > On Mon, Sep 19, 2022 at 9:43 AM Paulo Alcantara <pc@cjr.nz> wrote: >> >> Enzo Matsumiya <ematsumiya@suse.de> writes: >> >>> There's a race when cifs_readv_receive() might dequeue the mid, >>> and mid->callback(), called from demultiplex thread, will try to >>> access it to verify the signature before the mid is actually >>> released/deleted. >>> >>> Currently the signature verification fails, but the verification >>> shouldn't have happened at all because the mid was deleted because >>> of an error, and hence not really supposed to be passed to >>> ->callback(). There are no further errors because the mid is >>> effectivelly gone by the end of the callback. >>> >>> This patch checks if the mid doesn't have the MID_DELETED flag set (by >>> dequeue_mid()) right before trying to verify the signature. According to >>> my tests, trying to check it earlier, e.g. after the ->receive() call in >>> cifs_demultiplex_thread, will fail most of the time as dequeue_mid() >>> might not have been called yet. >>> >>> This behaviour can be seen in xfstests generic/465, for example, where >>> mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be >>> discarded, but instead have their signature computed, but mismatched. >>> >>> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> >>> --- >>> fs/cifs/cifssmb.c | 2 +- >>> fs/cifs/smb2pdu.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> Good catch! >> >> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > >
On 09/20, Tom Talpey wrote: >I guess I caught up out-of-order, replying to the other thread. > >Catching the race is good, but "dequeuing the MID" has nothing to >do with signing and should not be listed as justification. If >the message is being processed, e.g. returning the status field, >then the payload MUST be validated per the processing in 3.2.5.1.3. >This validation requires only a valid session, and the message itself. Fully agreed. >Apparently the code is storing the decryption status in the local >mid structure. That's the root-cause bug here. The signing validation >must not be skipped otherwise! Poking holes in security is never a >good approach. Can the decryption boolean be stored someplace else? This was not on a encrypted session, only mounted with sign option. (the decryption process happens in the parent if block of the ->receive() call, thus, taking a different route) I think I found the problem. In smb2_readv_callback: ---- 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 }; ---- This rqst assembling only considers the success case, where there _is_ data in rdata->pages. Since those fields in rdata are preset and ->pages pre-allocated in send (cifs_send_async_read), there was never an "obvious" failure here (e.g. NULL deref). The signature verification fails because it runs over the iov (SMB2 header + read rsp error struct) *and* the pages, where the page data is valid from the memory POV (allocated, >0 pages, etc), but it's certainly not from the signature check POV. I could confirm this by setting only the iov in rqst in the case of rdata->result != 0. But I'm still evaluating the best way to go through this a) by using rdata->result to choose which data to pass to signature check, but b) without breaking any other read scenarios, and c) without verifying the same message twice >Tom. > >On 9/20/2022 12:15 AM, Steve French wrote: >>merged into cifs-2.6.git for-next >> >>On Mon, Sep 19, 2022 at 9:43 AM Paulo Alcantara <pc@cjr.nz> wrote: >>> >>>Enzo Matsumiya <ematsumiya@suse.de> writes: >>> >>>>There's a race when cifs_readv_receive() might dequeue the mid, >>>>and mid->callback(), called from demultiplex thread, will try to >>>>access it to verify the signature before the mid is actually >>>>released/deleted. >>>> >>>>Currently the signature verification fails, but the verification >>>>shouldn't have happened at all because the mid was deleted because >>>>of an error, and hence not really supposed to be passed to >>>>->callback(). There are no further errors because the mid is >>>>effectivelly gone by the end of the callback. >>>> >>>>This patch checks if the mid doesn't have the MID_DELETED flag set (by >>>>dequeue_mid()) right before trying to verify the signature. According to >>>>my tests, trying to check it earlier, e.g. after the ->receive() call in >>>>cifs_demultiplex_thread, will fail most of the time as dequeue_mid() >>>>might not have been called yet. >>>> >>>>This behaviour can be seen in xfstests generic/465, for example, where >>>>mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be >>>>discarded, but instead have their signature computed, but mismatched. >>>> >>>>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> >>>>--- >>>> fs/cifs/cifssmb.c | 2 +- >>>> fs/cifs/smb2pdu.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>>Good catch! >>> >>>Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> I asked to drop this one because it was the wrong solution, but I think we'll still need to have a similar check/handling somewhere to prevent a NULL mid or mid with bogus data from being accessed. Will report back asap. Cheers, Enzo
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index addf3fc62aef..116f6afe33c6 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -1308,7 +1308,7 @@ cifs_readv_callback(struct mid_q_entry *mid) switch (mid->mid_state) { case MID_RESPONSE_RECEIVED: /* result already set, check signature */ - if (server->sign) { + if (server->sign && !(mid->mid_flags & MID_DELETED)) { int rc = 0; rc = cifs_verify_signature(&rqst, server, diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 5da0b596c8a0..394996c4f729 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -4136,7 +4136,7 @@ smb2_readv_callback(struct mid_q_entry *mid) credits.value = le16_to_cpu(shdr->CreditRequest); credits.instance = server->reconnect_instance; /* result already set, check signature */ - if (server->sign && !mid->decrypted) { + if (server->sign && !mid->decrypted && !(mid->mid_flags & MID_DELETED)) { int rc; rc = smb2_verify_signature(&rqst, server);
There's a race when cifs_readv_receive() might dequeue the mid, and mid->callback(), called from demultiplex thread, will try to access it to verify the signature before the mid is actually released/deleted. Currently the signature verification fails, but the verification shouldn't have happened at all because the mid was deleted because of an error, and hence not really supposed to be passed to ->callback(). There are no further errors because the mid is effectivelly gone by the end of the callback. This patch checks if the mid doesn't have the MID_DELETED flag set (by dequeue_mid()) right before trying to verify the signature. According to my tests, trying to check it earlier, e.g. after the ->receive() call in cifs_demultiplex_thread, will fail most of the time as dequeue_mid() might not have been called yet. This behaviour can be seen in xfstests generic/465, for example, where mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be discarded, but instead have their signature computed, but mismatched. Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> --- fs/cifs/cifssmb.c | 2 +- fs/cifs/smb2pdu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)