On Thu, Feb 11, 2021 at 07:35:33PM -0800, Drew Fustini wrote:

On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:

quoted

On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:

quoted

On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:

quoted

On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:

quoted

+	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "failed to copy buffer from userspace");
+		goto free_gname;
+	}
+	buf[len-1] = '\0';
+
+	ret = sscanf(buf, "%s %s", fname, gname);
+	if (ret != 2) {
+		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
+		goto free_gname;

We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
codes.  Normally we would write it like so:

	if (sscanf(buf, "%s %s", fname, gname) != 2) {
		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
		ret = -EINVAL;
		goto free_gname;
	}

I'm going to write a Smatch check for this today.

It's a pretty frequently used style:

$ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
327

Yeah.  That's true.  I looked through a couple of those and they were
fine.  (Sample size 2)  But the other format is more common.

$ git grep sscanf | grep = | wc -l
803

I have written a Smatch check to complain whenever we propogate the
return value from sscanf.  I'll let you know tomorrow how that goes.

I should write another check which says "On this error path, we know
sscanf was not equal to the value we wanted but we are still returning
success".

regards,
dan carpenter

Thank you for comments regarding sscanf().  And also thank you for the
LF mentorship session on smatch this morning.  It helped me understand
it much better.

Good deal!

The warning about propagating errors from sscanf caught a couple bugs.
The one about returning success if sscanf failed didn't catch anything.

The sscanf overflow patch didn't find anything either, but I think we've
had those bugs in the past and so I expect some in the future so I will
keep that one in my private tests without pushing it.

regards,
dan carpenter