| Message ID | 20210322202650.45776-1-george.mccollister@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e0c755a45f6fb6e81e3a62a94db0400ef0cdc046 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: don't assign an error value to tag_ops | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 27 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Mar 22, 2021 at 03:26:50PM -0500, George McCollister wrote: > Use a temporary variable to hold the return value from > dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving > an error value in dst->tag_ops can result in deferencing an invalid > pointer when a deferred switch configuration happens later. > > Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > --- Who dereferences the invalid pointer? dsa_tree_free I guess?
On Mon, Mar 22, 2021 at 3:46 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Mon, Mar 22, 2021 at 03:26:50PM -0500, George McCollister wrote: > > Use a temporary variable to hold the return value from > > dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving > > an error value in dst->tag_ops can result in deferencing an invalid > > pointer when a deferred switch configuration happens later. > > > > Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") > > > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > > --- > > Who dereferences the invalid pointer? dsa_tree_free I guess? I saw it occur just above on the following line the next time dsa_port_parse_cpu() is called: if (dst->tag_ops->proto != tag_protocol) { -George
On Mon, Mar 22, 2021 at 03:26:50PM -0500, George McCollister wrote: > Use a temporary variable to hold the return value from > dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving > an error value in dst->tag_ops can result in deferencing an invalid > pointer when a deferred switch configuration happens later. > > Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Just FYI, new lines aren't typically added between the various tags.
On 3/22/2021 1:26 PM, George McCollister wrote: > Use a temporary variable to hold the return value from > dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving > an error value in dst->tag_ops can result in deferencing an invalid > pointer when a deferred switch configuration happens later. > > Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") > > Signed-off-by: George McCollister <george.mccollister@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Mon, 22 Mar 2021 15:26:50 -0500 you wrote: > Use a temporary variable to hold the return value from > dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving > an error value in dst->tag_ops can result in deferencing an invalid > pointer when a deferred switch configuration happens later. > > Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") > > [...] Here is the summary with links: - [net] net: dsa: don't assign an error value to tag_ops https://git.kernel.org/netdev/net/c/e0c755a45f6f You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index eb709d988c54..8f9e35e1aa89 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -1068,6 +1068,7 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master) { struct dsa_switch *ds = dp->ds; struct dsa_switch_tree *dst = ds->dst; + const struct dsa_device_ops *tag_ops; enum dsa_tag_protocol tag_protocol; tag_protocol = dsa_get_tag_protocol(dp, master); @@ -1082,14 +1083,16 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master) * nothing to do here. */ } else { - dst->tag_ops = dsa_tag_driver_get(tag_protocol); - if (IS_ERR(dst->tag_ops)) { - if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT) + tag_ops = dsa_tag_driver_get(tag_protocol); + if (IS_ERR(tag_ops)) { + if (PTR_ERR(tag_ops) == -ENOPROTOOPT) return -EPROBE_DEFER; dev_warn(ds->dev, "No tagger for this switch\n"); dp->master = NULL; - return PTR_ERR(dst->tag_ops); + return PTR_ERR(tag_ops); } + + dst->tag_ops = tag_ops; } dp->master = master;
Use a temporary variable to hold the return value from dsa_tag_driver_get() instead of assigning it to dst->tag_ops. Leaving an error value in dst->tag_ops can result in deferencing an invalid pointer when a deferred switch configuration happens later. Fixes: 357f203bb3b5 ("net: dsa: keep a copy of the tagging protocol in the DSA switch tree") Signed-off-by: George McCollister <george.mccollister@gmail.com> --- net/dsa/dsa2.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)