| Message ID | 20241028-ncsi-fixes-v1-2-f0bcfaf6eb88@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ncsi: minor fixes | expand |
On Mon, Oct 28, 2024 at 01:06:57PM +0800, Jeremy Kerr wrote: > When constructing a netlink NCSI channel info message, we assume that > the hardware version field is nul-terminated, which may not be the case > for version name strings that are exactly 12 bytes. Is this defined by a standard? Does the standard allow non nul-terminated strings? Andrew
Hi Andrew, Thanks for checking this out. > Is this defined by a standard? Does the standard allow non > nul-terminated strings? Yes and yes. The pertinent wording of the spec is: The string is null terminated if the string is smaller than the field size However, regardless of what the spec says, we still don't want the strlen() in nla_put_string() to continue into arbitrary memory in the case there was no nul in the fw_name reported by the device. Cheers, Jeremy
On Tue, Oct 29, 2024 at 08:31:55AM +0800, Jeremy Kerr wrote: > Hi Andrew, > Thanks for checking this out. > > > > Is this defined by a standard? Does the standard allow non > > nul-terminated strings? > > > Yes and yes. The pertinent wording of the spec is: > > The string is null terminated if the string is smaller than the > field size > > However, regardless of what the spec says, we still don't want the > strlen() in nla_put_string() to continue into arbitrary memory in the > case there was no nul in the fw_name reported by the device. I agree with that, but i was thinking that if it was not allowed, we should be printing a warning telling the user to upgrade their buggy firmware. However, its not a bug. Are there any other strings which will need similar treatment? Andrew
Hi Andrew, > > However, regardless of what the spec says, we still don't want the > > strlen() in nla_put_string() to continue into arbitrary memory in > > the > > case there was no nul in the fw_name reported by the device. > > I agree with that, but i was thinking that if it was not allowed, we > should be printing a warning telling the user to upgrade their buggy > firmware. Gotchya. All fine there. > Are there any other strings which will need similar treatment? This is the only nla_put_string() in the ncsi code, and there are no other string-adjacent components of data represented in the spec (that I have come across, at least). Cheers, Jeremy
On Tue, Oct 29, 2024 at 12:06:58PM +0800, Jeremy Kerr wrote: > Hi Andrew, > > > > However, regardless of what the spec says, we still don't want the > > > strlen() in nla_put_string() to continue into arbitrary memory in > > > the > > > case there was no nul in the fw_name reported by the device. > > > > I agree with that, but i was thinking that if it was not allowed, we > > should be printing a warning telling the user to upgrade their buggy > > firmware. > > Gotchya. All fine there. > > > Are there any other strings which will need similar treatment? > > This is the only nla_put_string() in the ncsi code, and there are no > other string-adjacent components of data represented in the spec (that > I have come across, at least). It is worth mentioning all this in the commit message. It answers questions from reviewers before they ask the questions... Andrew
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c index 2f872d064396df55c2e213c525bae7740c12f62e..f2ba74537061ff616ce48a587969fd2270fb44c9 100644 --- a/net/ncsi/ncsi-netlink.c +++ b/net/ncsi/ncsi-netlink.c @@ -58,6 +58,8 @@ static int ncsi_write_channel_info(struct sk_buff *skb, struct ncsi_dev_priv *ndp, struct ncsi_channel *nc) { + const unsigned int fw_name_len = sizeof(nc->version.fw_name); + char fw_name[sizeof(nc->version.fw_name) + 1]; struct ncsi_channel_vlan_filter *ncf; struct ncsi_channel_mode *m; struct nlattr *vid_nest; @@ -73,7 +75,14 @@ static int ncsi_write_channel_info(struct sk_buff *skb, nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.major); nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MINOR, nc->version.minor); - nla_put_string(skb, NCSI_CHANNEL_ATTR_VERSION_STR, nc->version.fw_name); + + /* the fw_name string will only be nul-terminated if it is shorter + * than the 12-bytes available in the packet definition; ensure we have + * the correct terminator here. + */ + memcpy(fw_name, nc->version.fw_name, fw_name_len); + fw_name[fw_name_len] = '\0'; + nla_put_string(skb, NCSI_CHANNEL_ATTR_VERSION_STR, fw_name); vid_nest = nla_nest_start_noflag(skb, NCSI_CHANNEL_ATTR_VLAN_LIST); if (!vid_nest)
When constructing a netlink NCSI channel info message, we assume that the hardware version field is nul-terminated, which may not be the case for version name strings that are exactly 12 bytes. Build a proper nul-terminated buffer to use in nla_put_string() instead. Fixes: 955dc68cb9b2 ("net/ncsi: Add generic netlink family") Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- net/ncsi/ncsi-netlink.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)