linux-kernel - Re: [PATCH] rpmsg: virtio_rpmsg

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <92dc4403-8782-8dbf-b912-cc832ac748a7@ti.com>
Date:   Mon, 12 Aug 2019 12:36:04 -0400
From:   "Andrew F. Davis" <afd@...com>
To:     Suman Anna <s-anna@...com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     <linux-remoteproc@...r.kernel.org>,
        Loic Pallardy <loic.pallardy@...com>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

On 8/12/19 12:28 PM, Suman Anna wrote:
> On 8/12/19 10:47 AM, Andrew F. Davis wrote:
>> On 10/23/18 9:19 PM, Suman Anna wrote:
>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>> printing the vring buffer address. This prints only a hashed
>>> pointer even for previliged users. Use "%pK" instead so that
>>> the address can be printed during debug using kptr_restrict
>>> sysctl.
>>>
>>
>>
>> s/previliged/privileged
> 
> Bjorn,
> Can you fix this up when applying.
> 
>>
>> You describe what the code does, but not why you need this. %pK is used
>> for only about 1% of pointer printing, why do you want to leak this
>> address to userspace at all?
> 
> Andrew,
> Default behavior of %pK is same as %p, but it does allow you to control
> the print. The reason is clearly mentioned in the last sentence in the
> patch description.
> 


Let me rephrase then, why would you ever set 'kptr_restrict' to anything
other than 0, or why do you want to be able to leak this address to
userspace at all? If the answer is just because you can, then all 12,000
instances of %p in kernel could be converted for the same reason.

Andrew


> regards
> Suman
> 
>>
>> Andrew
>>
>>
>>> Signed-off-by: Suman Anna <s-anna@...com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index f29dee731026..1345f373a1a0 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>  		goto vqs_del;
>>>  	}
>>>  
>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>  		bufs_va, &vrp->bufs_dma);
>>>  
>>>  	/* half of the buffers is dedicated for RX */
>>>
>