| Message ID | 20170314183804.13788-1-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Krzysztof, I was wondering about the benefit of this. From a quick look these are all messages that end up in the kernel log / dmesg. IIRC %pK does nothing there, since dmest_restrict is supposed to be used to deny an unpriviliged user the access to the kernel log. Or am I missing something here? - Tobias Krzysztof Kozlowski wrote: > Printing raw kernel pointers might reveal information which sometimes we > try to hide (e.g. with Kernel Address Space Layout Randomization). Use > the "%pK" format so these pointers will be hidden for unprivileged > users. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- > drivers/gpu/drm/exynos/exynos_drm_fimc.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_gsc.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_ipp.c | 22 +++++++++++----------- > drivers/gpu/drm/exynos/exynos_drm_rotator.c | 2 +- > 6 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 812e2ec0761d..202526b20b64 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -979,7 +979,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi, > bool first = !xfer->tx_done; > u32 reg; > > - dev_dbg(dev, "< xfer %p: tx len %u, done %u, rx len %u, done %u\n", > + dev_dbg(dev, "< xfer %pK: tx len %u, done %u, rx len %u, done %u\n", > xfer, length, xfer->tx_done, xfer->rx_len, xfer->rx_done); > > if (length > DSI_TX_FIFO_SIZE) > @@ -1177,7 +1177,7 @@ static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi) > spin_unlock_irqrestore(&dsi->transfer_lock, flags); > > dev_dbg(dsi->dev, > - "> xfer %p, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", > + "> xfer %pK, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", > xfer, xfer->packet.payload_length, xfer->tx_done, xfer->rx_len, > xfer->rx_done); > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > index 95871577015d..5b18b5c5fdf2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > @@ -1695,7 +1695,7 @@ static int fimc_probe(struct platform_device *pdev) > goto err_put_clk; > } > > - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); > + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); > > spin_lock_init(&ctx->lock); > platform_set_drvdata(pdev, ctx); > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index 4c28f7ffcc4d..55a1579d11b3 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -218,7 +218,7 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev, > return ERR_PTR(ret); > } > > - DRM_DEBUG_KMS("created file object = %p\n", obj->filp); > + DRM_DEBUG_KMS("created file object = %pK\n", obj->filp); > > return exynos_gem; > } > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > index bef57987759d..0506b2b17ac1 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > @@ -1723,7 +1723,7 @@ static int gsc_probe(struct platform_device *pdev) > return ret; > } > > - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); > + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); > > mutex_init(&ctx->lock); > platform_set_drvdata(pdev, ctx); > diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c > index 9c84ee76f18a..3edda18cc2d2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c > @@ -208,7 +208,7 @@ static struct exynos_drm_ippdrv *ipp_find_drv_by_handle(u32 prop_id) > * e.g PAUSE state, queue buf, command control. > */ > list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) { > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", count++, ippdrv); > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", count++, ippdrv); > > mutex_lock(&ippdrv->cmd_lock); > list_for_each_entry(c_node, &ippdrv->cmd_list, list) { > @@ -388,7 +388,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data, > } > property->prop_id = ret; > > - DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%p]\n", > + DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%pK]\n", > property->prop_id, property->cmd, ippdrv); > > /* stored property information and ippdrv in private data */ > @@ -518,7 +518,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev, > { > int i; > > - DRM_DEBUG_KMS("node[%p]\n", m_node); > + DRM_DEBUG_KMS("node[%pK]\n", m_node); > > if (!m_node) { > DRM_ERROR("invalid dequeue node.\n"); > @@ -562,7 +562,7 @@ static struct drm_exynos_ipp_mem_node > m_node->buf_id = qbuf->buf_id; > INIT_LIST_HEAD(&m_node->list); > > - DRM_DEBUG_KMS("m_node[%p]ops_id[%d]\n", m_node, qbuf->ops_id); > + DRM_DEBUG_KMS("m_node[%pK]ops_id[%d]\n", m_node, qbuf->ops_id); > DRM_DEBUG_KMS("prop_id[%d]buf_id[%d]\n", qbuf->prop_id, m_node->buf_id); > > for_each_ipp_planar(i) { > @@ -659,7 +659,7 @@ static void ipp_put_event(struct drm_exynos_ipp_cmd_node *c_node, > > mutex_lock(&c_node->event_lock); > list_for_each_entry_safe(e, te, &c_node->event_list, base.link) { > - DRM_DEBUG_KMS("count[%d]e[%p]\n", count++, e); > + DRM_DEBUG_KMS("count[%d]e[%pK]\n", count++, e); > > /* > * qbuf == NULL condition means all event deletion. > @@ -750,7 +750,7 @@ static struct drm_exynos_ipp_mem_node > > /* find memory node from memory list */ > list_for_each_entry(m_node, head, list) { > - DRM_DEBUG_KMS("count[%d]m_node[%p]\n", count++, m_node); > + DRM_DEBUG_KMS("count[%d]m_node[%pK]\n", count++, m_node); > > /* compare buffer id */ > if (m_node->buf_id == qbuf->buf_id) > @@ -767,7 +767,7 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv, > struct exynos_drm_ipp_ops *ops = NULL; > int ret = 0; > > - DRM_DEBUG_KMS("node[%p]\n", m_node); > + DRM_DEBUG_KMS("node[%pK]\n", m_node); > > if (!m_node) { > DRM_ERROR("invalid queue node.\n"); > @@ -1232,7 +1232,7 @@ static int ipp_start_property(struct exynos_drm_ippdrv *ippdrv, > m_node = list_first_entry(head, > struct drm_exynos_ipp_mem_node, list); > > - DRM_DEBUG_KMS("m_node[%p]\n", m_node); > + DRM_DEBUG_KMS("m_node[%pK]\n", m_node); > > ret = ipp_set_mem_node(ippdrv, c_node, m_node); > if (ret) { > @@ -1601,7 +1601,7 @@ static int ipp_subdrv_probe(struct drm_device *drm_dev, struct device *dev) > } > ippdrv->prop_list.ipp_id = ret; > > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]ipp_id[%d]\n", > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]ipp_id[%d]\n", > count++, ippdrv, ret); > > /* store parent device for node */ > @@ -1659,7 +1659,7 @@ static int ipp_subdrv_open(struct drm_device *drm_dev, struct device *dev, > > file_priv->ipp_dev = dev; > > - DRM_DEBUG_KMS("done priv[%p]\n", dev); > + DRM_DEBUG_KMS("done priv[%pK]\n", dev); > > return 0; > } > @@ -1676,7 +1676,7 @@ static void ipp_subdrv_close(struct drm_device *drm_dev, struct device *dev, > mutex_lock(&ippdrv->cmd_lock); > list_for_each_entry_safe(c_node, tc_node, > &ippdrv->cmd_list, list) { > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", > count++, ippdrv); > > if (c_node->filp == file) { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c > index 6591e406084c..79282a820ecc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c > @@ -748,7 +748,7 @@ static int rotator_probe(struct platform_device *pdev) > goto err_ippdrv_register; > } > > - DRM_DEBUG_KMS("ippdrv[%p]\n", ippdrv); > + DRM_DEBUG_KMS("ippdrv[%pK]\n", ippdrv); > > platform_set_drvdata(pdev, rot); > >
On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: > Hello Krzysztof, > > I was wondering about the benefit of this. From a quick look these are > all messages that end up in the kernel log / dmesg. > > IIRC %pK does nothing there, since dmest_restrict is supposed to be used > to deny an unpriviliged user the access to the kernel log. > > Or am I missing something here? These are regular printks so depending on kernel options (e.g. dynamic debug, drm.debug) these might be printed also in the console. Of course we could argue then if access to one of the consoles is worth securing. Actually, I think that we should get rid of printing of these kernel pointers entirely... Best regards, Krzysztof
Krzysztof Kozlowski wrote: > On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: >> Hello Krzysztof, >> >> I was wondering about the benefit of this. From a quick look these are >> all messages that end up in the kernel log / dmesg. >> >> IIRC %pK does nothing there, since dmest_restrict is supposed to be used >> to deny an unpriviliged user the access to the kernel log. >> >> Or am I missing something here? > > These are regular printks so depending on kernel options (e.g. dynamic > debug, drm.debug) these might be printed also in the console. Of course > we could argue then if access to one of the consoles is worth > securing. This here suggests otherwise. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sysctl/kernel.txt#n388 I have not tested this, but IIRC %pK is not honored by the kernel logging infrastucture. That's why dmesg_restrict is there. Correct me if I'm wrong. - Tobias > Actually, I think that we should get rid of printing of these kernel > pointers entirely... > > > Best regards, > Krzysztof > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tue, Mar 14, 2017 at 08:17:35PM +0100, Tobias Jakobi wrote: > Krzysztof Kozlowski wrote: > > On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: > >> Hello Krzysztof, > >> > >> I was wondering about the benefit of this. From a quick look these are > >> all messages that end up in the kernel log / dmesg. > >> > >> IIRC %pK does nothing there, since dmest_restrict is supposed to be used > >> to deny an unpriviliged user the access to the kernel log. > >> > >> Or am I missing something here? > > > > These are regular printks so depending on kernel options (e.g. dynamic > > debug, drm.debug) these might be printed also in the console. Of course > > we could argue then if access to one of the consoles is worth > > securing. > This here suggests otherwise. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sysctl/kernel.txt#n388 > > I have not tested this, but IIRC %pK is not honored by the kernel > logging infrastucture. That's why dmesg_restrict is there. > > Correct me if I'm wrong. The %pK will not help for dmesg or /proc/kmsg but it will help for console (/dev/ttySACN, ttySN etc) because effectively it uses the same vsprintf()/pointer() functions. As I said, we could argue whether securing console is worth... usually attacker having access to it has also physical access to the machine so everything gets easier... Best regards, Krzysztof
Krzysztof Kozlowski wrote: > On Tue, Mar 14, 2017 at 08:17:35PM +0100, Tobias Jakobi wrote: >> Krzysztof Kozlowski wrote: >>> On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: >>>> Hello Krzysztof, >>>> >>>> I was wondering about the benefit of this. From a quick look these are >>>> all messages that end up in the kernel log / dmesg. >>>> >>>> IIRC %pK does nothing there, since dmest_restrict is supposed to be used >>>> to deny an unpriviliged user the access to the kernel log. >>>> >>>> Or am I missing something here? >>> >>> These are regular printks so depending on kernel options (e.g. dynamic >>> debug, drm.debug) these might be printed also in the console. Of course >>> we could argue then if access to one of the consoles is worth >>> securing. >> This here suggests otherwise. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sysctl/kernel.txt#n388 >> >> I have not tested this, but IIRC %pK is not honored by the kernel >> logging infrastucture. That's why dmesg_restrict is there. >> >> Correct me if I'm wrong. > > The %pK will not help for dmesg or /proc/kmsg but it will help for > console (/dev/ttySACN, ttySN etc) because effectively it uses the same > vsprintf()/pointer() functions. Thanks for the explanation, I didn't know that there was a difference there. In that case, looks good to me. > As I said, we could argue whether securing console is worth... usually > attacker having access to it has also physical access to the machine so > everything gets easier... Still, putting %pK there certainly doesn't hurt. - Tobias > > Best regards, > Krzysztof > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Merged. Thanks, Inki Dae 2017년 03월 15일 03:38에 Krzysztof Kozlowski 이(가) 쓴 글: > Printing raw kernel pointers might reveal information which sometimes we > try to hide (e.g. with Kernel Address Space Layout Randomization). Use > the "%pK" format so these pointers will be hidden for unprivileged > users. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- > drivers/gpu/drm/exynos/exynos_drm_fimc.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_gsc.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_ipp.c | 22 +++++++++++----------- > drivers/gpu/drm/exynos/exynos_drm_rotator.c | 2 +- > 6 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 812e2ec0761d..202526b20b64 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -979,7 +979,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi, > bool first = !xfer->tx_done; > u32 reg; > > - dev_dbg(dev, "< xfer %p: tx len %u, done %u, rx len %u, done %u\n", > + dev_dbg(dev, "< xfer %pK: tx len %u, done %u, rx len %u, done %u\n", > xfer, length, xfer->tx_done, xfer->rx_len, xfer->rx_done); > > if (length > DSI_TX_FIFO_SIZE) > @@ -1177,7 +1177,7 @@ static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi) > spin_unlock_irqrestore(&dsi->transfer_lock, flags); > > dev_dbg(dsi->dev, > - "> xfer %p, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", > + "> xfer %pK, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", > xfer, xfer->packet.payload_length, xfer->tx_done, xfer->rx_len, > xfer->rx_done); > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > index 95871577015d..5b18b5c5fdf2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > @@ -1695,7 +1695,7 @@ static int fimc_probe(struct platform_device *pdev) > goto err_put_clk; > } > > - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); > + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); > > spin_lock_init(&ctx->lock); > platform_set_drvdata(pdev, ctx); > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index 4c28f7ffcc4d..55a1579d11b3 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -218,7 +218,7 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev, > return ERR_PTR(ret); > } > > - DRM_DEBUG_KMS("created file object = %p\n", obj->filp); > + DRM_DEBUG_KMS("created file object = %pK\n", obj->filp); > > return exynos_gem; > } > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > index bef57987759d..0506b2b17ac1 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > @@ -1723,7 +1723,7 @@ static int gsc_probe(struct platform_device *pdev) > return ret; > } > > - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); > + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); > > mutex_init(&ctx->lock); > platform_set_drvdata(pdev, ctx); > diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c > index 9c84ee76f18a..3edda18cc2d2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c > @@ -208,7 +208,7 @@ static struct exynos_drm_ippdrv *ipp_find_drv_by_handle(u32 prop_id) > * e.g PAUSE state, queue buf, command control. > */ > list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) { > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", count++, ippdrv); > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", count++, ippdrv); > > mutex_lock(&ippdrv->cmd_lock); > list_for_each_entry(c_node, &ippdrv->cmd_list, list) { > @@ -388,7 +388,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data, > } > property->prop_id = ret; > > - DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%p]\n", > + DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%pK]\n", > property->prop_id, property->cmd, ippdrv); > > /* stored property information and ippdrv in private data */ > @@ -518,7 +518,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev, > { > int i; > > - DRM_DEBUG_KMS("node[%p]\n", m_node); > + DRM_DEBUG_KMS("node[%pK]\n", m_node); > > if (!m_node) { > DRM_ERROR("invalid dequeue node.\n"); > @@ -562,7 +562,7 @@ static struct drm_exynos_ipp_mem_node > m_node->buf_id = qbuf->buf_id; > INIT_LIST_HEAD(&m_node->list); > > - DRM_DEBUG_KMS("m_node[%p]ops_id[%d]\n", m_node, qbuf->ops_id); > + DRM_DEBUG_KMS("m_node[%pK]ops_id[%d]\n", m_node, qbuf->ops_id); > DRM_DEBUG_KMS("prop_id[%d]buf_id[%d]\n", qbuf->prop_id, m_node->buf_id); > > for_each_ipp_planar(i) { > @@ -659,7 +659,7 @@ static void ipp_put_event(struct drm_exynos_ipp_cmd_node *c_node, > > mutex_lock(&c_node->event_lock); > list_for_each_entry_safe(e, te, &c_node->event_list, base.link) { > - DRM_DEBUG_KMS("count[%d]e[%p]\n", count++, e); > + DRM_DEBUG_KMS("count[%d]e[%pK]\n", count++, e); > > /* > * qbuf == NULL condition means all event deletion. > @@ -750,7 +750,7 @@ static struct drm_exynos_ipp_mem_node > > /* find memory node from memory list */ > list_for_each_entry(m_node, head, list) { > - DRM_DEBUG_KMS("count[%d]m_node[%p]\n", count++, m_node); > + DRM_DEBUG_KMS("count[%d]m_node[%pK]\n", count++, m_node); > > /* compare buffer id */ > if (m_node->buf_id == qbuf->buf_id) > @@ -767,7 +767,7 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv, > struct exynos_drm_ipp_ops *ops = NULL; > int ret = 0; > > - DRM_DEBUG_KMS("node[%p]\n", m_node); > + DRM_DEBUG_KMS("node[%pK]\n", m_node); > > if (!m_node) { > DRM_ERROR("invalid queue node.\n"); > @@ -1232,7 +1232,7 @@ static int ipp_start_property(struct exynos_drm_ippdrv *ippdrv, > m_node = list_first_entry(head, > struct drm_exynos_ipp_mem_node, list); > > - DRM_DEBUG_KMS("m_node[%p]\n", m_node); > + DRM_DEBUG_KMS("m_node[%pK]\n", m_node); > > ret = ipp_set_mem_node(ippdrv, c_node, m_node); > if (ret) { > @@ -1601,7 +1601,7 @@ static int ipp_subdrv_probe(struct drm_device *drm_dev, struct device *dev) > } > ippdrv->prop_list.ipp_id = ret; > > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]ipp_id[%d]\n", > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]ipp_id[%d]\n", > count++, ippdrv, ret); > > /* store parent device for node */ > @@ -1659,7 +1659,7 @@ static int ipp_subdrv_open(struct drm_device *drm_dev, struct device *dev, > > file_priv->ipp_dev = dev; > > - DRM_DEBUG_KMS("done priv[%p]\n", dev); > + DRM_DEBUG_KMS("done priv[%pK]\n", dev); > > return 0; > } > @@ -1676,7 +1676,7 @@ static void ipp_subdrv_close(struct drm_device *drm_dev, struct device *dev, > mutex_lock(&ippdrv->cmd_lock); > list_for_each_entry_safe(c_node, tc_node, > &ippdrv->cmd_list, list) { > - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", > + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", > count++, ippdrv); > > if (c_node->filp == file) { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c > index 6591e406084c..79282a820ecc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c > @@ -748,7 +748,7 @@ static int rotator_probe(struct platform_device *pdev) > goto err_ippdrv_register; > } > > - DRM_DEBUG_KMS("ippdrv[%p]\n", ippdrv); > + DRM_DEBUG_KMS("ippdrv[%pK]\n", ippdrv); > > platform_set_drvdata(pdev, rot); > >
Hi Tobias, On 14.03.2017 21:41, Tobias Jakobi wrote: > Krzysztof Kozlowski wrote: >> On Tue, Mar 14, 2017 at 08:17:35PM +0100, Tobias Jakobi wrote: >>> Krzysztof Kozlowski wrote: >>>> On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: >>>>> Hello Krzysztof, >>>>> >>>>> I was wondering about the benefit of this. From a quick look these are >>>>> all messages that end up in the kernel log / dmesg. >>>>> >>>>> IIRC %pK does nothing there, since dmest_restrict is supposed to be used >>>>> to deny an unpriviliged user the access to the kernel log. >>>>> >>>>> Or am I missing something here? >>>> These are regular printks so depending on kernel options (e.g. dynamic >>>> debug, drm.debug) these might be printed also in the console. Of course >>>> we could argue then if access to one of the consoles is worth >>>> securing. >>> This here suggests otherwise. >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sysctl/kernel.txt#n388 >>> >>> I have not tested this, but IIRC %pK is not honored by the kernel >>> logging infrastucture. That's why dmesg_restrict is there. >>> >>> Correct me if I'm wrong. >> The %pK will not help for dmesg or /proc/kmsg but it will help for >> console (/dev/ttySACN, ttySN etc) because effectively it uses the same >> vsprintf()/pointer() functions. > Thanks for the explanation, I didn't know that there was a difference > there. In that case, looks good to me. > > Just to clarify %pK: Documentation/printk-formats.txt: %pK 0x01234567 or 0x0123456789abcdef For printing kernel pointers which should be hidden from unprivileged users. The behaviour of %pK depends on the kptr_restrict sysctl - see Documentation/sysctl/kernel.txt for more details. Documentation/sysctl/kernel.txt: kptr_restrict: This toggle indicates whether restrictions are placed on exposing kernel addresses via /proc and other interfaces. When kptr_restrict is set to (0), the default, there are no restrictions. When kptr_restrict is set to (1), kernel pointers printed using the %pK format specifier will be replaced with 0's unless the user has CAP_SYSLOG and effective user and group ids are equal to the real ids. This is because %pK checks are done at read() time rather than open() time, so if permissions are elevated between the open() and the read() (e.g via a setuid binary) then %pK will not leak kernel pointers to unprivileged users. Note, this is a temporary solution only. The correct long-term solution is to do the permission checks at open() time. Consider removing world read permissions from files that use %pK, and using dmesg_restrict to protect against uses of %pK in dmesg(8) if leaking kernel pointer values to unprivileged users is a concern. When kptr_restrict is set to (2), kernel pointers printed using %pK will be replaced with 0's regardless of privileges. --- Regards Andrzej
Hello Andrzej, note that i had already pointed Krzysztof to that documentation in my previous mail. - Tobias Andrzej Hajda wrote: > Hi Tobias, > > On 14.03.2017 21:41, Tobias Jakobi wrote: >> Krzysztof Kozlowski wrote: >>> On Tue, Mar 14, 2017 at 08:17:35PM +0100, Tobias Jakobi wrote: >>>> Krzysztof Kozlowski wrote: >>>>> On Tue, Mar 14, 2017 at 08:01:41PM +0100, Tobias Jakobi wrote: >>>>>> Hello Krzysztof, >>>>>> >>>>>> I was wondering about the benefit of this. From a quick look these are >>>>>> all messages that end up in the kernel log / dmesg. >>>>>> >>>>>> IIRC %pK does nothing there, since dmest_restrict is supposed to be used >>>>>> to deny an unpriviliged user the access to the kernel log. >>>>>> >>>>>> Or am I missing something here? >>>>> These are regular printks so depending on kernel options (e.g. dynamic >>>>> debug, drm.debug) these might be printed also in the console. Of course >>>>> we could argue then if access to one of the consoles is worth >>>>> securing. >>>> This here suggests otherwise. >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sysctl/kernel.txt#n388 >>>> >>>> I have not tested this, but IIRC %pK is not honored by the kernel >>>> logging infrastucture. That's why dmesg_restrict is there. >>>> >>>> Correct me if I'm wrong. >>> The %pK will not help for dmesg or /proc/kmsg but it will help for >>> console (/dev/ttySACN, ttySN etc) because effectively it uses the same >>> vsprintf()/pointer() functions. >> Thanks for the explanation, I didn't know that there was a difference >> there. In that case, looks good to me. >> >> > > Just to clarify %pK: > > Documentation/printk-formats.txt: > %pK 0x01234567 or 0x0123456789abcdef > > For printing kernel pointers which should be hidden from > unprivileged > users. The behaviour of %pK depends on the kptr_restrict sysctl > - see > Documentation/sysctl/kernel.txt for more details. > > Documentation/sysctl/kernel.txt: > > kptr_restrict: > > This toggle indicates whether restrictions are placed on > exposing kernel addresses via /proc and other interfaces. > > When kptr_restrict is set to (0), the default, there are no restrictions. > > When kptr_restrict is set to (1), kernel pointers printed using the %pK > format specifier will be replaced with 0's unless the user has CAP_SYSLOG > and effective user and group ids are equal to the real ids. This is > because %pK checks are done at read() time rather than open() time, so > if permissions are elevated between the open() and the read() (e.g via > a setuid binary) then %pK will not leak kernel pointers to unprivileged > users. Note, this is a temporary solution only. The correct long-term > solution is to do the permission checks at open() time. Consider removing > world read permissions from files that use %pK, and using dmesg_restrict > to protect against uses of %pK in dmesg(8) if leaking kernel pointer > values to unprivileged users is a concern. > > When kptr_restrict is set to (2), kernel pointers printed using > %pK will be replaced with 0's regardless of privileges. > --- > > Regards > Andrzej >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 812e2ec0761d..202526b20b64 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -979,7 +979,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi, bool first = !xfer->tx_done; u32 reg; - dev_dbg(dev, "< xfer %p: tx len %u, done %u, rx len %u, done %u\n", + dev_dbg(dev, "< xfer %pK: tx len %u, done %u, rx len %u, done %u\n", xfer, length, xfer->tx_done, xfer->rx_len, xfer->rx_done); if (length > DSI_TX_FIFO_SIZE) @@ -1177,7 +1177,7 @@ static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi) spin_unlock_irqrestore(&dsi->transfer_lock, flags); dev_dbg(dsi->dev, - "> xfer %p, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", + "> xfer %pK, tx_len %zu, tx_done %u, rx_len %u, rx_done %u\n", xfer, xfer->packet.payload_length, xfer->tx_done, xfer->rx_len, xfer->rx_done); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c index 95871577015d..5b18b5c5fdf2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c @@ -1695,7 +1695,7 @@ static int fimc_probe(struct platform_device *pdev) goto err_put_clk; } - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); spin_lock_init(&ctx->lock); platform_set_drvdata(pdev, ctx); diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 4c28f7ffcc4d..55a1579d11b3 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -218,7 +218,7 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev, return ERR_PTR(ret); } - DRM_DEBUG_KMS("created file object = %p\n", obj->filp); + DRM_DEBUG_KMS("created file object = %pK\n", obj->filp); return exynos_gem; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c index bef57987759d..0506b2b17ac1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c @@ -1723,7 +1723,7 @@ static int gsc_probe(struct platform_device *pdev) return ret; } - DRM_DEBUG_KMS("id[%d]ippdrv[%p]\n", ctx->id, ippdrv); + DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv); mutex_init(&ctx->lock); platform_set_drvdata(pdev, ctx); diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c index 9c84ee76f18a..3edda18cc2d2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c @@ -208,7 +208,7 @@ static struct exynos_drm_ippdrv *ipp_find_drv_by_handle(u32 prop_id) * e.g PAUSE state, queue buf, command control. */ list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) { - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", count++, ippdrv); + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", count++, ippdrv); mutex_lock(&ippdrv->cmd_lock); list_for_each_entry(c_node, &ippdrv->cmd_list, list) { @@ -388,7 +388,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data, } property->prop_id = ret; - DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%p]\n", + DRM_DEBUG_KMS("created prop_id[%d]cmd[%d]ippdrv[%pK]\n", property->prop_id, property->cmd, ippdrv); /* stored property information and ippdrv in private data */ @@ -518,7 +518,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev, { int i; - DRM_DEBUG_KMS("node[%p]\n", m_node); + DRM_DEBUG_KMS("node[%pK]\n", m_node); if (!m_node) { DRM_ERROR("invalid dequeue node.\n"); @@ -562,7 +562,7 @@ static struct drm_exynos_ipp_mem_node m_node->buf_id = qbuf->buf_id; INIT_LIST_HEAD(&m_node->list); - DRM_DEBUG_KMS("m_node[%p]ops_id[%d]\n", m_node, qbuf->ops_id); + DRM_DEBUG_KMS("m_node[%pK]ops_id[%d]\n", m_node, qbuf->ops_id); DRM_DEBUG_KMS("prop_id[%d]buf_id[%d]\n", qbuf->prop_id, m_node->buf_id); for_each_ipp_planar(i) { @@ -659,7 +659,7 @@ static void ipp_put_event(struct drm_exynos_ipp_cmd_node *c_node, mutex_lock(&c_node->event_lock); list_for_each_entry_safe(e, te, &c_node->event_list, base.link) { - DRM_DEBUG_KMS("count[%d]e[%p]\n", count++, e); + DRM_DEBUG_KMS("count[%d]e[%pK]\n", count++, e); /* * qbuf == NULL condition means all event deletion. @@ -750,7 +750,7 @@ static struct drm_exynos_ipp_mem_node /* find memory node from memory list */ list_for_each_entry(m_node, head, list) { - DRM_DEBUG_KMS("count[%d]m_node[%p]\n", count++, m_node); + DRM_DEBUG_KMS("count[%d]m_node[%pK]\n", count++, m_node); /* compare buffer id */ if (m_node->buf_id == qbuf->buf_id) @@ -767,7 +767,7 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv, struct exynos_drm_ipp_ops *ops = NULL; int ret = 0; - DRM_DEBUG_KMS("node[%p]\n", m_node); + DRM_DEBUG_KMS("node[%pK]\n", m_node); if (!m_node) { DRM_ERROR("invalid queue node.\n"); @@ -1232,7 +1232,7 @@ static int ipp_start_property(struct exynos_drm_ippdrv *ippdrv, m_node = list_first_entry(head, struct drm_exynos_ipp_mem_node, list); - DRM_DEBUG_KMS("m_node[%p]\n", m_node); + DRM_DEBUG_KMS("m_node[%pK]\n", m_node); ret = ipp_set_mem_node(ippdrv, c_node, m_node); if (ret) { @@ -1601,7 +1601,7 @@ static int ipp_subdrv_probe(struct drm_device *drm_dev, struct device *dev) } ippdrv->prop_list.ipp_id = ret; - DRM_DEBUG_KMS("count[%d]ippdrv[%p]ipp_id[%d]\n", + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]ipp_id[%d]\n", count++, ippdrv, ret); /* store parent device for node */ @@ -1659,7 +1659,7 @@ static int ipp_subdrv_open(struct drm_device *drm_dev, struct device *dev, file_priv->ipp_dev = dev; - DRM_DEBUG_KMS("done priv[%p]\n", dev); + DRM_DEBUG_KMS("done priv[%pK]\n", dev); return 0; } @@ -1676,7 +1676,7 @@ static void ipp_subdrv_close(struct drm_device *drm_dev, struct device *dev, mutex_lock(&ippdrv->cmd_lock); list_for_each_entry_safe(c_node, tc_node, &ippdrv->cmd_list, list) { - DRM_DEBUG_KMS("count[%d]ippdrv[%p]\n", + DRM_DEBUG_KMS("count[%d]ippdrv[%pK]\n", count++, ippdrv); if (c_node->filp == file) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 6591e406084c..79282a820ecc 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c @@ -748,7 +748,7 @@ static int rotator_probe(struct platform_device *pdev) goto err_ippdrv_register; } - DRM_DEBUG_KMS("ippdrv[%p]\n", ippdrv); + DRM_DEBUG_KMS("ippdrv[%pK]\n", ippdrv); platform_set_drvdata(pdev, rot);
Printing raw kernel pointers might reveal information which sometimes we try to hide (e.g. with Kernel Address Space Layout Randomization). Use the "%pK" format so these pointers will be hidden for unprivileged users. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++-- drivers/gpu/drm/exynos/exynos_drm_fimc.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gsc.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_ipp.c | 22 +++++++++++----------- drivers/gpu/drm/exynos/exynos_drm_rotator.c | 2 +- 6 files changed, 17 insertions(+), 17 deletions(-)