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
