Re: [PATCH] cifs: verify signature only for valid responses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/18/2022 8:21 PM, Enzo Matsumiya wrote:
Hi Tom,

On 09/17, Tom Talpey wrote:
<snip>
Wait, we process the message *before* we check the signature??? Apart
from inspecting the MID and verifying it's a response to a request we
made, there isn't a lot to cause such an error. See 3.2.5.1.3.

You're right. By processing I actually meant "parsing" done right after
receive, but even that doesn't have many failure spots.

I found that the mids with STATUS_END_OF_FILE are being discarded,
apparently as per 3.2.5.11:

   If the Status field of the SMB2 header of the response indicates an
   error, the client MUST return the received status code to the calling
   application.

I don't think it follows that the signature MUST NOT be validated.
That processing is fundamental, and is independent of returning
results. 3.2.5.1.3 has only three exceptions, and STATUS_END_OF_FILE
isn't one of them.

What I found is that mid->callback() (smb2_readv_callback()) was being
called from another thread, so even though the mid had been dequeued by
mid->receive() earlier, smb2_readv_callback() was treating it as a
valid (non-NULL), existing (mid_state == MID_RESPONSE_RECEIVED) mid.

That certainly sounds like another bug. Why are two threads processing
the MID? Is this an async response?

 From this perspective, it makes sense to me to skip the signature
verification when the mid wasn't supposed to be there in the first

That's not what is documented. Only if the MID is 0xFFFFFFFFFFFFFFFF.

place, but if we consider that other messages with status !=
STATUS_SUCCESS have their signatures correctly computed (apparently),
then I'd guess there's something wrong with computing signatures for
STATUS_END_OF_FILE responses.

I agree.

Tom.

Sent this just now:
https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@xxxxxxx/T/#u

I'd appreciate your, and Cc'd folks', feedback.


Cheers,

Enzo




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux