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@xxxxxx> wrote:
Enzo Matsumiya <ematsumiya@xxxxxxx> 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@xxxxxxx>
---
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@xxxxxx>
