Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

[Dominique Martinet]

piaojun wrote on Fri, Aug 03, 2018:
> We'd better reach an agreement about the patch fix.

The way I read Greg's comment was that he agreed to the proposed changes
and is waiting for a new version.


I'm writing a longer reply than I should because I don't like people
saying strncmp is safe just because it's strncmp, feel free to skim
through the rest as it's just ranting.


> In my opinion, replacing strlen(chan->tag) with a local variable
> sounds reasonable,

So we agree here

> and changing strncmp to strcmp may be little beneficial, as strcmp is more
> dangerours such as buffer-flow.

strcmp is more dangerous for buffer-overflow if you're comparing
"unsafe" non-null terminated strings.
This isn't the case here as you've constructed chan->tag yourself and
you rely on it being null-terminated by calling strlen() on it yourself.

strncmp(x, y, strlen(y)+1) is at best awkward, but it's a false sense of
security if you think this is any better than strcmp here. It implies that:
 - y is null-terminated (for strlen() to work)
 - x is either null-terminated or longer than y

Here, x is the devname argument to p9_virtio_create, which comes
straight from the mount syscall with "copy_mount_string", using
strndup_user, which returns a null-terminated string or an error.

(the code is currently not safe if it returns an error, I'm sending
another mail about it right after this one as we already have a partial
fix)


strcmp(x, y) on the other hand assumes that x and y are null-terminated
in this case, which is the same assumptions you have, so is strictly as
"safe" as strncmp used that way.
(it could also assume that one is null terminated and the other one is
at least as long as that, e.g. when comparing a "char buf[42]" to the
constant "foo" then we don't care if buf is null-terminated)


Thanks,
-- 
Dominique