linux-kernel - Re: qrtr/mhi: NULL-deref with in-kernel pd-mapper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Zyzoh0zv1Z7LDfjW@hovoldconsulting.com>
Date: Thu, 7 Nov 2024 17:19:19 +0100
From: Johan Hovold <johan@...nel.org>
To: Chris Lew <quic_clew@...cinc.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
	Qiang Yu <quic_qianyu@...cinc.com>,
	Jeffrey Hugo <quic_jhugo@...cinc.com>,
	Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	Konrad Dybcio <konradybcio@...nel.org>, mhi@...ts.linux.dev,
	linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: qrtr/mhi: NULL-deref with in-kernel pd-mapper

On Tue, Nov 05, 2024 at 10:26:40AM -0800, Chris Lew wrote:
> On 11/4/2024 9:08 PM, Johan Hovold wrote:
> > On Mon, Nov 04, 2024 at 04:26:15PM -0800, Chris Lew wrote:

> >> This looks like the null pointer would happen if qrtr tried to send
> >> before mhi_channel_prepare() is called.

> >> I think we have a patch that might fix this, let me dig it up and send
> >> it out.
> > 
> > Would that patch still help?
> > 
> > 	https://lore.kernel.org/lkml/20241104-qrtr_mhi-v1-1-79adf7e3bba5@quicinc.com/
> 
> Yea this is the exact patch I had in mind, didnt realize the patch was 
> already sent a while back.

Heh, that's a bit of an understatement. Apparently the fix was posted
three years ago, but no one followed up with a v2:

	https://lore.kernel.org/lkml/1626831778-31796-1-git-send-email-bbhatt@codeaurora.org/

> > I naively tried adding a sleep after registering the endpoint, but that
> > is at least not sufficient to trigger the NULL-deref.
> 
> Looking at the callstack, this is broadcasting a NEW_SERVER notification 
> from qrtr_ns. I think you can force this by starting and stopping some 
> qmi service with the added sleep. Do you have tqftpserv or diag-router 
> in your environment? Those will open qmi services, so starting and 
> stopping those will cause the new_server broadcast in qrtr_ns.

No, neither tqftpserv or diag-router are used here, but after digging
through the code it seems my hunch about this being related to the
in-kernel pd-mapper was correct.

The qrtr worker, qrtr_ns_worker(), is called when the in-kernel
pd-mapper adds the server, and processing the QRTR_TYPE_NEW_SERVER
command eventually ends up in mhi_gen_tre() for the modem:

[    9.026694] qcom_pdm_start - adding server
[    9.034684] ctrl_cmd_new_server - service = 0x40, instance = 0x101, node_id = 1, port = 0
[    9.042155] mhi-pci-generic 0005:01:00.0: mhi_gen_tre - buf_info = ffff800080d4d038, offset_of(buf_info->used) = 34
[   10.669996] Call trace:
[   10.787734]  mhi_gen_tre+0x218/0x270 [mhi]
[   10.804727]  mhi_queue+0x74/0x194 [mhi]
[   10.804730]  mhi_queue_skb+0x5c/0x8c [mhi]
[   10.804732]  qcom_mhi_qrtr_send+0x6c/0x160 [qrtr_mhi]
[   10.804734]  qrtr_node_enqueue+0xd0/0x4a0 [qrtr]
[   10.804736]  qrtr_bcast_enqueue+0x78/0xe8 [qrtr]
[   10.804737]  qrtr_sendmsg+0x15c/0x33c [qrtr]
[   10.804739]  sock_sendmsg+0xc0/0xec
[   10.804742]  kernel_sendmsg+0x30/0x40
[   10.804743]  service_announce_new+0xbc/0x1c4 [qrtr]
[   10.804745]  qrtr_ns_worker+0x754/0x7d4 [qrtr]

And I can indeed imagine that leading to the NULL deref in case the
endpoint is registered before being fully set up.

Johan