| Message ID | 20241116125415.30799-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: adv7511: ADV7535 fixes | expand |
Hi Biju, Thank you for the patch. On Sat, Nov 16, 2024 at 12:54:10PM +0000, Biju Das wrote: > The host_node pointer was assigned and freed in adv7533_parse_dt(), and > later, adv7533_attach_dsi() used the same. Fix this use-after-free issue > with the below changes: > > 1. Drop host_node from struct adv7511 and instead use a local pointer in > adv7511_probe(). > 2. Update adv7533_parse_dt() to return the host_node. > 3. Pass the host_node as a parameter to adv7533_attach_dsi(). > 4. Call of_node_put() after use. > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") > Cc: stable@vger.kernel.org > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > Changes in v4: > - Updated commit description. > - Dropped host_node from struct adv7511 and instead used a local pointer > in probe(). Also freeing of host_node pointer after use is done in > probe(). > Changes in v3: > - Replace __free construct with readable of_node_put(). > Changes in v2: > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > - Dropped Archit Taneja invalid Mail address > --- > drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +++--- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 22 ++++++++++++++------ > drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +++++++++--------- > 3 files changed, 29 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h > index ec0b7f3d889c..9f3fae7cc597 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -383,7 +383,6 @@ struct adv7511 { > struct regulator_bulk_data *supplies; > > /* ADV7533 DSI RX related params */ > - struct device_node *host_node; > struct mipi_dsi_device *dsi; > u8 num_dsi_lanes; > bool use_timing_gen; > @@ -417,8 +416,9 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, > const struct drm_display_mode *mode); > int adv7533_patch_registers(struct adv7511 *adv); > int adv7533_patch_cec_registers(struct adv7511 *adv); > -int adv7533_attach_dsi(struct adv7511 *adv); > -int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv); > +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node *host_node); > +struct device_node *adv7533_parse_dt(struct device_node *np, > + struct adv7511 *adv); > > #ifdef CONFIG_DRM_I2C_ADV7511_AUDIO > int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511); > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index eb5919b38263..3f1f309791a5 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1209,6 +1209,7 @@ static int adv7511_parse_dt(struct device_node *np, > static int adv7511_probe(struct i2c_client *i2c) > { > struct adv7511_link_config link_config; > + struct device_node *host_node = NULL; > struct adv7511 *adv7511; > struct device *dev = &i2c->dev; > unsigned int val; > @@ -1233,12 +1234,17 @@ static int adv7511_probe(struct i2c_client *i2c) > if (ret && ret != -ENODEV) > return ret; > > - if (adv7511->info->link_config) > + if (adv7511->info->link_config) { > ret = adv7511_parse_dt(dev->of_node, &link_config); > - else > - ret = adv7533_parse_dt(dev->of_node, adv7511); > - if (ret) > - return ret; > + if (ret) > + return ret; > + } > + > + if (adv7511->info->has_dsi) { > + host_node = adv7533_parse_dt(dev->of_node, adv7511); > + if (IS_ERR(host_node)) > + return PTR_ERR(host_node); > + } > > ret = adv7511_init_regulators(adv7511); > if (ret) host_node is leaked here. > @@ -1343,9 +1349,11 @@ static int adv7511_probe(struct i2c_client *i2c) > } > > if (adv7511->info->has_dsi) { > - ret = adv7533_attach_dsi(adv7511); > + ret = adv7533_attach_dsi(adv7511, host_node); > if (ret) > goto err_unregister_audio; > + > + of_node_put(host_node); > } > > return 0; > @@ -1362,6 +1370,8 @@ static int adv7511_probe(struct i2c_client *i2c) > err_i2c_unregister_edid: > i2c_unregister_device(adv7511->i2c_edid); > uninit_regulators: > + if (host_node) > + of_node_put(host_node); The error label and the error handling code are now out of sync, making the code harder to read and more error-prone. Error handling is why I proposed keeping of_node in the adv7511 structure, and calling of_node_put() in adv7511_remove() and at the end of the error handling path in adv7511_probe(). > adv7511_uninit_regulators(adv7511); > > return ret; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > index 4481489aaf5e..5d0e55ef4028 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -131,7 +131,7 @@ int adv7533_patch_cec_registers(struct adv7511 *adv) > ARRAY_SIZE(adv7533_cec_fixed_registers)); > } > > -int adv7533_attach_dsi(struct adv7511 *adv) > +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node *host_node) > { > struct device *dev = &adv->i2c_main->dev; > struct mipi_dsi_host *host; > @@ -142,7 +142,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) > .node = NULL, > }; > > - host = of_find_mipi_dsi_host_by_node(adv->host_node); > + host = of_find_mipi_dsi_host_by_node(host_node); > if (!host) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to find dsi host\n"); > @@ -166,22 +166,22 @@ int adv7533_attach_dsi(struct adv7511 *adv) > return 0; > } > > -int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > +struct device_node *adv7533_parse_dt(struct device_node *np, > + struct adv7511 *adv) > { > + struct device_node *host_node; > u32 num_lanes; > > of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); > > if (num_lanes < 1 || num_lanes > 4) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > adv->num_dsi_lanes = num_lanes; > > - adv->host_node = of_graph_get_remote_node(np, 0, 0); > - if (!adv->host_node) > - return -ENODEV; > - > - of_node_put(adv->host_node); > + host_node = of_graph_get_remote_node(np, 0, 0); > + if (!host_node) > + return ERR_PTR(-ENODEV); > > adv->use_timing_gen = !of_property_read_bool(np, > "adi,disable-timing-generator"); > @@ -190,5 +190,5 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > adv->rgb = true; > adv->embedded_sync = false; > > - return 0; > + return host_node; > }
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Laurent Pinchart > Sent: 16 November 2024 15:32 > Subject: Re: [PATCH v4 1/3] drm: adv7511: Fix use-after-free in adv7533_attach_dsi() > > Hi Biju, > > Thank you for the patch. > > On Sat, Nov 16, 2024 at 12:54:10PM +0000, Biju Das wrote: > > The host_node pointer was assigned and freed in adv7533_parse_dt(), > > and later, adv7533_attach_dsi() used the same. Fix this use-after-free > > issue with the below changes: > > > > 1. Drop host_node from struct adv7511 and instead use a local pointer in > > adv7511_probe(). > > 2. Update adv7533_parse_dt() to return the host_node. > > 3. Pass the host_node as a parameter to adv7533_attach_dsi(). > > 4. Call of_node_put() after use. > > > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") > > Cc: stable@vger.kernel.org > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > Changes in v4: > > - Updated commit description. > > - Dropped host_node from struct adv7511 and instead used a local pointer > > in probe(). Also freeing of host_node pointer after use is done in > > probe(). > > Changes in v3: > > - Replace __free construct with readable of_node_put(). > > Changes in v2: > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > - Dropped Archit Taneja invalid Mail address > > --- > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +++--- > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 22 ++++++++++++++------ > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +++++++++--------- > > 3 files changed, 29 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > index ec0b7f3d889c..9f3fae7cc597 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > @@ -383,7 +383,6 @@ struct adv7511 { > > struct regulator_bulk_data *supplies; > > > > /* ADV7533 DSI RX related params */ > > - struct device_node *host_node; > > struct mipi_dsi_device *dsi; > > u8 num_dsi_lanes; > > bool use_timing_gen; > > @@ -417,8 +416,9 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, > > const struct drm_display_mode *mode); int > > adv7533_patch_registers(struct adv7511 *adv); int > > adv7533_patch_cec_registers(struct adv7511 *adv); -int > > adv7533_attach_dsi(struct adv7511 *adv); -int adv7533_parse_dt(struct > > device_node *np, struct adv7511 *adv); > > +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node > > +*host_node); struct device_node *adv7533_parse_dt(struct device_node *np, > > + struct adv7511 *adv); > > > > #ifdef CONFIG_DRM_I2C_ADV7511_AUDIO > > int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511); > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > index eb5919b38263..3f1f309791a5 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > @@ -1209,6 +1209,7 @@ static int adv7511_parse_dt(struct device_node > > *np, static int adv7511_probe(struct i2c_client *i2c) { > > struct adv7511_link_config link_config; > > + struct device_node *host_node = NULL; > > struct adv7511 *adv7511; > > struct device *dev = &i2c->dev; > > unsigned int val; > > @@ -1233,12 +1234,17 @@ static int adv7511_probe(struct i2c_client *i2c) > > if (ret && ret != -ENODEV) > > return ret; > > > > - if (adv7511->info->link_config) > > + if (adv7511->info->link_config) { > > ret = adv7511_parse_dt(dev->of_node, &link_config); > > - else > > - ret = adv7533_parse_dt(dev->of_node, adv7511); > > - if (ret) > > - return ret; > > + if (ret) > > + return ret; > > + } > > + > > + if (adv7511->info->has_dsi) { > > + host_node = adv7533_parse_dt(dev->of_node, adv7511); > > + if (IS_ERR(host_node)) > > + return PTR_ERR(host_node); > > + } > > > > ret = adv7511_init_regulators(adv7511); > > if (ret) > > host_node is leaked here. Oops. Missed it. > > > @@ -1343,9 +1349,11 @@ static int adv7511_probe(struct i2c_client *i2c) > > } > > > > if (adv7511->info->has_dsi) { > > - ret = adv7533_attach_dsi(adv7511); > > + ret = adv7533_attach_dsi(adv7511, host_node); > > if (ret) > > goto err_unregister_audio; > > + > > + of_node_put(host_node); > > } > > > > return 0; > > @@ -1362,6 +1370,8 @@ static int adv7511_probe(struct i2c_client *i2c) > > err_i2c_unregister_edid: > > i2c_unregister_device(adv7511->i2c_edid); > > uninit_regulators: > > + if (host_node) > > + of_node_put(host_node); > > The error label and the error handling code are now out of sync, making the code harder to read and > more error-prone. > > Error handling is why I proposed keeping of_node in the adv7511 structure, and calling of_node_put() > in adv7511_remove() and at the end of the error handling path in adv7511_probe(). OK got it, Then will keep the of_node in the adv7511 structure to make the error label and error handling code in sync. Cheers, Biju > > > adv7511_uninit_regulators(adv7511); > > > > return ret; > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > index 4481489aaf5e..5d0e55ef4028 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > @@ -131,7 +131,7 @@ int adv7533_patch_cec_registers(struct adv7511 *adv) > > ARRAY_SIZE(adv7533_cec_fixed_registers)); > > } > > > > -int adv7533_attach_dsi(struct adv7511 *adv) > > +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node > > +*host_node) > > { > > struct device *dev = &adv->i2c_main->dev; > > struct mipi_dsi_host *host; > > @@ -142,7 +142,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) > > .node = NULL, > > }; > > > > - host = of_find_mipi_dsi_host_by_node(adv->host_node); > > + host = of_find_mipi_dsi_host_by_node(host_node); > > if (!host) > > return dev_err_probe(dev, -EPROBE_DEFER, > > "failed to find dsi host\n"); @@ -166,22 +166,22 @@ int > > adv7533_attach_dsi(struct adv7511 *adv) > > return 0; > > } > > > > -int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > +struct device_node *adv7533_parse_dt(struct device_node *np, > > + struct adv7511 *adv) > > { > > + struct device_node *host_node; > > u32 num_lanes; > > > > of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); > > > > if (num_lanes < 1 || num_lanes > 4) > > - return -EINVAL; > > + return ERR_PTR(-EINVAL); > > > > adv->num_dsi_lanes = num_lanes; > > > > - adv->host_node = of_graph_get_remote_node(np, 0, 0); > > - if (!adv->host_node) > > - return -ENODEV; > > - > > - of_node_put(adv->host_node); > > + host_node = of_graph_get_remote_node(np, 0, 0); > > + if (!host_node) > > + return ERR_PTR(-ENODEV); > > > > adv->use_timing_gen = !of_property_read_bool(np, > > "adi,disable-timing-generator"); @@ -190,5 +190,5 @@ int > > adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > adv->rgb = true; > > adv->embedded_sync = false; > > > > - return 0; > > + return host_node; > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index ec0b7f3d889c..9f3fae7cc597 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -383,7 +383,6 @@ struct adv7511 { struct regulator_bulk_data *supplies; /* ADV7533 DSI RX related params */ - struct device_node *host_node; struct mipi_dsi_device *dsi; u8 num_dsi_lanes; bool use_timing_gen; @@ -417,8 +416,9 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, const struct drm_display_mode *mode); int adv7533_patch_registers(struct adv7511 *adv); int adv7533_patch_cec_registers(struct adv7511 *adv); -int adv7533_attach_dsi(struct adv7511 *adv); -int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv); +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node *host_node); +struct device_node *adv7533_parse_dt(struct device_node *np, + struct adv7511 *adv); #ifdef CONFIG_DRM_I2C_ADV7511_AUDIO int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511); diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index eb5919b38263..3f1f309791a5 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1209,6 +1209,7 @@ static int adv7511_parse_dt(struct device_node *np, static int adv7511_probe(struct i2c_client *i2c) { struct adv7511_link_config link_config; + struct device_node *host_node = NULL; struct adv7511 *adv7511; struct device *dev = &i2c->dev; unsigned int val; @@ -1233,12 +1234,17 @@ static int adv7511_probe(struct i2c_client *i2c) if (ret && ret != -ENODEV) return ret; - if (adv7511->info->link_config) + if (adv7511->info->link_config) { ret = adv7511_parse_dt(dev->of_node, &link_config); - else - ret = adv7533_parse_dt(dev->of_node, adv7511); - if (ret) - return ret; + if (ret) + return ret; + } + + if (adv7511->info->has_dsi) { + host_node = adv7533_parse_dt(dev->of_node, adv7511); + if (IS_ERR(host_node)) + return PTR_ERR(host_node); + } ret = adv7511_init_regulators(adv7511); if (ret) @@ -1343,9 +1349,11 @@ static int adv7511_probe(struct i2c_client *i2c) } if (adv7511->info->has_dsi) { - ret = adv7533_attach_dsi(adv7511); + ret = adv7533_attach_dsi(adv7511, host_node); if (ret) goto err_unregister_audio; + + of_node_put(host_node); } return 0; @@ -1362,6 +1370,8 @@ static int adv7511_probe(struct i2c_client *i2c) err_i2c_unregister_edid: i2c_unregister_device(adv7511->i2c_edid); uninit_regulators: + if (host_node) + of_node_put(host_node); adv7511_uninit_regulators(adv7511); return ret; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 4481489aaf5e..5d0e55ef4028 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -131,7 +131,7 @@ int adv7533_patch_cec_registers(struct adv7511 *adv) ARRAY_SIZE(adv7533_cec_fixed_registers)); } -int adv7533_attach_dsi(struct adv7511 *adv) +int adv7533_attach_dsi(struct adv7511 *adv, struct device_node *host_node) { struct device *dev = &adv->i2c_main->dev; struct mipi_dsi_host *host; @@ -142,7 +142,7 @@ int adv7533_attach_dsi(struct adv7511 *adv) .node = NULL, }; - host = of_find_mipi_dsi_host_by_node(adv->host_node); + host = of_find_mipi_dsi_host_by_node(host_node); if (!host) return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n"); @@ -166,22 +166,22 @@ int adv7533_attach_dsi(struct adv7511 *adv) return 0; } -int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) +struct device_node *adv7533_parse_dt(struct device_node *np, + struct adv7511 *adv) { + struct device_node *host_node; u32 num_lanes; of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); if (num_lanes < 1 || num_lanes > 4) - return -EINVAL; + return ERR_PTR(-EINVAL); adv->num_dsi_lanes = num_lanes; - adv->host_node = of_graph_get_remote_node(np, 0, 0); - if (!adv->host_node) - return -ENODEV; - - of_node_put(adv->host_node); + host_node = of_graph_get_remote_node(np, 0, 0); + if (!host_node) + return ERR_PTR(-ENODEV); adv->use_timing_gen = !of_property_read_bool(np, "adi,disable-timing-generator"); @@ -190,5 +190,5 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) adv->rgb = true; adv->embedded_sync = false; - return 0; + return host_node; }
The host_node pointer was assigned and freed in adv7533_parse_dt(), and later, adv7533_attach_dsi() used the same. Fix this use-after-free issue with the below changes: 1. Drop host_node from struct adv7511 and instead use a local pointer in adv7511_probe(). 2. Update adv7533_parse_dt() to return the host_node. 3. Pass the host_node as a parameter to adv7533_attach_dsi(). 4. Call of_node_put() after use. Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") Cc: stable@vger.kernel.org Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- Changes in v4: - Updated commit description. - Dropped host_node from struct adv7511 and instead used a local pointer in probe(). Also freeing of host_node pointer after use is done in probe(). Changes in v3: - Replace __free construct with readable of_node_put(). Changes in v2: - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. - Dropped Archit Taneja invalid Mail address --- drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +++--- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 22 ++++++++++++++------ drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +++++++++--------- 3 files changed, 29 insertions(+), 19 deletions(-)