[PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Sagi Grimberg]

From Alon:
"Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
a malicious user can cause a UAF and a double free, which may lead to
RCE (may also lead to an LPE in case the attacker already has local
privileges)."

Hence, when a queue initialization fails after the ahash requests are
allocated, it is guaranteed that the queue removal async work will be
called, hence leave the deallocation to the queue removal.

Also, be extra careful not to continue processing the socket, so set
queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.

Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/tcp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 97d07488072d..d840f996eb82 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -372,6 +372,7 @@ static void nvmet_tcp_fatal_error(struct nvmet_tcp_queue *queue)
 
 static void nvmet_tcp_socket_error(struct nvmet_tcp_queue *queue, int status)
 {
+	queue->rcv_state = NVMET_TCP_RECV_ERR;
 	if (status == -EPIPE || status == -ECONNRESET)
 		kernel_sock_shutdown(queue->sock, SHUT_RDWR);
 	else
@@ -910,15 +911,11 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
 	iov.iov_len = sizeof(*icresp);
 	ret = kernel_sendmsg(queue->sock, &msg, &iov, 1, iov.iov_len);
 	if (ret < 0)
-		goto free_crypto;
+		return ret; /* queue removal will cleanup */
 
 	queue->state = NVMET_TCP_Q_LIVE;
 	nvmet_prepare_receive_pdu(queue);
 	return 0;
-free_crypto:
-	if (queue->hdr_digest || queue->data_digest)
-		nvmet_tcp_free_crypto(queue);
-	return ret;
 }
 
 static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
-- 
2.41.0

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Chaitanya Kulkarni]

On 10/2/2023 3:54 AM, Sagi Grimberg wrote:
>  From Alon:
> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
> a malicious user can cause a UAF and a double free, which may lead to
> RCE (may also lead to an LPE in case the attacker already has local
> privileges)."
> 
> Hence, when a queue initialization fails after the ahash requests are
> allocated, it is guaranteed that the queue removal async work will be
> called, hence leave the deallocation to the queue removal.
> 
> Also, be extra careful not to continue processing the socket, so set
> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
> 
> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---

Prior to this patch nvmet_tcp_free_crypto has only two callers [1],
does it makes sense to in-line nvmet_tcp_free_crypto() in only
remaining caller nvmet_tcp_release_queue_work() ?

Irrespective of that looks good to me.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1]

1 tcp.c nvmet_tcp_handle_icreq        920 nvmet_tcp_free_crypto(queue); 

2 tcp.c nvmet_tcp_release_queue_work 1499 nvmet_tcp_free_crypto(queue); 
 
 

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Sagi Grimberg]

>>   From Alon:
>> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
>> a malicious user can cause a UAF and a double free, which may lead to
>> RCE (may also lead to an LPE in case the attacker already has local
>> privileges)."
>>
>> Hence, when a queue initialization fails after the ahash requests are
>> allocated, it is guaranteed that the queue removal async work will be
>> called, hence leave the deallocation to the queue removal.
>>
>> Also, be extra careful not to continue processing the socket, so set
>> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
>>
>> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
>> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
> 
> Prior to this patch nvmet_tcp_free_crypto has only two callers [1],
> does it makes sense to in-line nvmet_tcp_free_crypto() in only
> remaining caller nvmet_tcp_release_queue_work() ?

Its also the opposite of nvmet_tcp_alloc_crypto. Possible to fold both
but I think its also fine to keep.

> 
> Irrespective of that looks good to me.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Thanks

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[sj]

Hello,

On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:

> From Alon:
> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
> a malicious user can cause a UAF and a double free, which may lead to
> RCE (may also lead to an LPE in case the attacker already has local
> privileges)."
> 
> Hence, when a queue initialization fails after the ahash requests are
> allocated, it is guaranteed that the queue removal async work will be
> called, hence leave the deallocation to the queue removal.
> 
> Also, be extra careful not to continue processing the socket, so set
> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
> 
> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Would it be better to add Fixes: and Cc: stable lines?


Thanks,
SJ

> ---
>  drivers/nvme/target/tcp.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 97d07488072d..d840f996eb82 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -372,6 +372,7 @@ static void nvmet_tcp_fatal_error(struct nvmet_tcp_queue *queue)
>  
>  static void nvmet_tcp_socket_error(struct nvmet_tcp_queue *queue, int status)
>  {
> +	queue->rcv_state = NVMET_TCP_RECV_ERR;
>  	if (status == -EPIPE || status == -ECONNRESET)
>  		kernel_sock_shutdown(queue->sock, SHUT_RDWR);
>  	else
> @@ -910,15 +911,11 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
>  	iov.iov_len = sizeof(*icresp);
>  	ret = kernel_sendmsg(queue->sock, &msg, &iov, 1, iov.iov_len);
>  	if (ret < 0)
> -		goto free_crypto;
> +		return ret; /* queue removal will cleanup */
>  
>  	queue->state = NVMET_TCP_Q_LIVE;
>  	nvmet_prepare_receive_pdu(queue);
>  	return 0;
> -free_crypto:
> -	if (queue->hdr_digest || queue->data_digest)
> -		nvmet_tcp_free_crypto(queue);
> -	return ret;
>  }
>  
>  static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
> -- 
> 2.41.0
> 
> 

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Sagi Grimberg]

> Hello,
> 
> On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:
> 
>>  From Alon:
>> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
>> a malicious user can cause a UAF and a double free, which may lead to
>> RCE (may also lead to an LPE in case the attacker already has local
>> privileges)."
>>
>> Hence, when a queue initialization fails after the ahash requests are
>> allocated, it is guaranteed that the queue removal async work will be
>> called, hence leave the deallocation to the queue removal.
>>
>> Also, be extra careful not to continue processing the socket, so set
>> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
>>
>> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
>> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> 
> Would it be better to add Fixes: and Cc: stable lines?

This issue existed since the introduction of the driver, I am not sure
it applies cleanly that far back...

I figured that the description and Reported-by tag will trigger stable
kernel pick up...

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Greg KH]

On Wed, Oct 04, 2023 at 12:41:30PM +0300, Sagi Grimberg wrote:
> 
> > Hello,
> > 
> > On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:
> > 
> > >  From Alon:
> > > "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
> > > a malicious user can cause a UAF and a double free, which may lead to
> > > RCE (may also lead to an LPE in case the attacker already has local
> > > privileges)."
> > > 
> > > Hence, when a queue initialization fails after the ahash requests are
> > > allocated, it is guaranteed that the queue removal async work will be
> > > called, hence leave the deallocation to the queue removal.
> > > 
> > > Also, be extra careful not to continue processing the socket, so set
> > > queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
> > > 
> > > Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> > > Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > 
> > Would it be better to add Fixes: and Cc: stable lines?
> 
> This issue existed since the introduction of the driver, I am not sure
> it applies cleanly that far back...
> 
> I figured that the description and Reported-by tag will trigger stable
> kernel pick up...

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Sagi Grimberg]

>>> Hello,
>>>
>>> On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:
>>>
>>>>   From Alon:
>>>> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
>>>> a malicious user can cause a UAF and a double free, which may lead to
>>>> RCE (may also lead to an LPE in case the attacker already has local
>>>> privileges)."
>>>>
>>>> Hence, when a queue initialization fails after the ahash requests are
>>>> allocated, it is guaranteed that the queue removal async work will be
>>>> called, hence leave the deallocation to the queue removal.
>>>>
>>>> Also, be extra careful not to continue processing the socket, so set
>>>> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
>>>>
>>>> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
>>>> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>
>>> Would it be better to add Fixes: and Cc: stable lines?
>>
>> This issue existed since the introduction of the driver, I am not sure
>> it applies cleanly that far back...
>>
>> I figured that the description and Reported-by tag will trigger stable
>> kernel pick up...
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

I could have sworn to have seen patches that did not have stable CCd
nor a Fixes tag and was picked up for stable kernels :)
But I guess those were either hallucinations or someone sending patches
to stable...

I can resend with CC to stable.

Thanks.

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[SeongJae Park]

On Wed, 4 Oct 2023 16:25:13 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:

> 
> >>> Hello,
> >>>
> >>> On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:
> >>>
> >>>>   From Alon:
> >>>> "Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
> >>>> a malicious user can cause a UAF and a double free, which may lead to
> >>>> RCE (may also lead to an LPE in case the attacker already has local
> >>>> privileges)."
> >>>>
> >>>> Hence, when a queue initialization fails after the ahash requests are
> >>>> allocated, it is guaranteed that the queue removal async work will be
> >>>> called, hence leave the deallocation to the queue removal.
> >>>>
> >>>> Also, be extra careful not to continue processing the socket, so set
> >>>> queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
> >>>>
> >>>> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> >>>> Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
> >>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> >>>
> >>> Would it be better to add Fixes: and Cc: stable lines?
> >>
> >> This issue existed since the introduction of the driver, I am not sure
> >> it applies cleanly that far back...

Based on the rule[1], I think people who need this fix on the old kernel would
do that.  I'd just say adding Fixes: would help it, so better than nothing.

[1] https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

> >>
> >> I figured that the description and Reported-by tag will trigger stable
> >> kernel pick up...
> > 
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > </formletter>
> 
> I could have sworn to have seen patches that did not have stable CCd
> nor a Fixes tag and was picked up for stable kernels :)

I could also swear.  Stable kernel maintainers are great at finding fixes on
their own.  But I think adding those could help them avoiding any mistake, and
therefore better than nothing, again.

> But I guess those were either hallucinations or someone sending patches
> to stable...
> 
> I can resend with CC to stable.

Thank you :)


Thanks,
SJ

> 
> Thanks.
> 
> 

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Sasha Levin]

On Wed, Oct 04, 2023 at 04:25:13PM +0300, Sagi Grimberg wrote:
>
>>>>Hello,
>>>>
>>>>On Mon, 2 Oct 2023 13:54:28 +0300 Sagi Grimberg <sagi@grimberg.me> wrote:
>>>>
>>>>>  From Alon:
>>>>>"Due to a logical bug in the NVMe-oF/TCP subsystem in the Linux kernel,
>>>>>a malicious user can cause a UAF and a double free, which may lead to
>>>>>RCE (may also lead to an LPE in case the attacker already has local
>>>>>privileges)."
>>>>>
>>>>>Hence, when a queue initialization fails after the ahash requests are
>>>>>allocated, it is guaranteed that the queue removal async work will be
>>>>>called, hence leave the deallocation to the queue removal.
>>>>>
>>>>>Also, be extra careful not to continue processing the socket, so set
>>>>>queue rcv_state to NVMET_TCP_RECV_ERR upon a socket error.
>>>>>
>>>>>Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
>>>>>Tested-by: Alon Zahavi <zahavi.alon@gmail.com>
>>>>>Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>
>>>>Would it be better to add Fixes: and Cc: stable lines?
>>>
>>>This issue existed since the introduction of the driver, I am not sure
>>>it applies cleanly that far back...
>>>
>>>I figured that the description and Reported-by tag will trigger stable
>>>kernel pick up...
>>
>><formletter>
>>
>>This is not the correct way to submit patches for inclusion in the
>>stable kernel tree.  Please read:
>>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>for how to do this properly.
>>
>></formletter>
>
>I could have sworn to have seen patches that did not have stable CCd
>nor a Fixes tag and was picked up for stable kernels :)
>But I guess those were either hallucinations or someone sending patches
>to stable...

That happens, but there are no guarantees around it.

If you really want for a patch to land in stable, and want to know if
for some reason it didn't, the only way to do it is with an explicit
stable tag. Otherwise it's just "best effort".

-- 
Thanks,
Sasha

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvmet-tcp: Fix a possible UAF in queue intialization setup

[Keith Busch]

Applied for nvme-6.6.