Re: [PATCH] cifs: check if mid was deleted in async read callback

[Enzo Matsumiya <ematsumiya@suse.de>]

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