| Message ID | 20230510-tpic-bearer_name_validate-v1-1-016d882e4e99@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] tipic: guard against buffer overrun in bearer_name_validate() | expand |
On Wed, May 10, 2023 at 02:48:11PM +0200, Simon Horman wrote: > Smatch reports that copying media_name and if_name to name_parts may > overwrite the destination. > > .../bearer.c:166 bearer_name_validate() error: strcpy() 'media_name' too large for 'name_parts->media_name' (32 vs 16) > .../bearer.c:167 bearer_name_validate() error: strcpy() 'if_name' too large for 'name_parts->if_name' (1010102 vs 16) > > This does seem to be the case, although perhaps it never occurs in > practice due to well formed input. > > Guard against this possibility by using strscpy() and failing if > truncation occurs. > The existing code is safe as you say. I still feel like strscpy() is better than strcpy() unless it's a fast path. The Smatch strlen() code is not very good. 137 static int bearer_name_validate(const char *name, 138 struct tipc_bearer_names *name_parts) 139 { 140 char name_copy[TIPC_MAX_BEARER_NAME]; 141 char *media_name; 142 char *if_name; 143 u32 media_len; 144 u32 if_len; 145 146 /* copy bearer name & ensure length is OK */ 147 if (strscpy(name_copy, name, TIPC_MAX_BEARER_NAME) < 0) 148 return 0; Smatch tracks strlen() but it tracks it as a maximum. So here smatch says that strlen name_copy is 31. TODO 1: It should really track it as a range 0-31. TODO 2: Create module to track strlen if the strlen is stored in a variable. TODO 3: Create a new module to say if a string is NUL terminated. (not related to this bug) TODO 4: Create a new module to say if a string is not NUL terminated. (not related) 149 150 /* ensure all component parts of bearer name are present */ 151 media_name = name_copy; 152 if_name = strchr(media_name, ':'); TODO 5: Add strchr() handling to Smatch *DONE in my tree* 153 if (if_name == NULL) 154 return 0; 155 *(if_name++) = 0; TODO: 6: Before Smatch saw ++ and set strlen to unknown. It should be set to 29. *DONE in my tree* TODO 7: Create a new module to say that if_name points to the middle of the media_name buffer. The *if_name = 0; means that media_name strlen is now 29. 156 media_len = if_name - media_name; TODO 8: This is saying that "media_len = strlen(media_name) + 1". Tricky to do. 157 if_len = strlen(if_name) + 1; TODO 9: The existing code has a bug here because it thinks if_name is length 29 so that means if_len is 30. I hacked around this in my tree so now it says it is 1-30 but TODO 1 is the correct fix. 158 159 /* validate component parts of bearer name */ 160 if ((media_len <= 1) || (media_len > TIPC_MAX_MEDIA_NAME) || 161 (if_len <= 1) || (if_len > TIPC_MAX_IF_NAME)) 162 return 0; I think TODO 2 would make this work automatically for if_name. 163 164 /* return bearer name components, if necessary */ 165 if (name_parts) { 166 strcpy(name_parts->media_name, media_name); 167 strcpy(name_parts->if_name, if_name); 168 } 169 return 1; 170 } It's a lot of work to handle this correctly. Most of it is not complicated, except TODO 7-8 which are very hard. regards, dan carpenter
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 35cac7733fd3..a82cd8f351a5 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -163,8 +163,12 @@ static int bearer_name_validate(const char *name, /* return bearer name components, if necessary */ if (name_parts) { - strcpy(name_parts->media_name, media_name); - strcpy(name_parts->if_name, if_name); + if (strscpy(name_parts->media_name, media_name, + TIPC_MAX_MEDIA_NAME) < 0) + return 0; + if (strscpy(name_parts->if_name, if_name, + TIPC_MAX_IF_NAME) < 0) + return 0; } return 1; }
Smatch reports that copying media_name and if_name to name_parts may overwrite the destination. .../bearer.c:166 bearer_name_validate() error: strcpy() 'media_name' too large for 'name_parts->media_name' (32 vs 16) .../bearer.c:167 bearer_name_validate() error: strcpy() 'if_name' too large for 'name_parts->if_name' (1010102 vs 16) This does seem to be the case, although perhaps it never occurs in practice due to well formed input. Guard against this possibility by using strscpy() and failing if truncation occurs. Compile tested only. Fixes: b97bf3fd8f6a ("[TIPC] Initial merge") Signed-off-by: Simon Horman <horms@kernel.org> --- net/tipc/bearer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)