| Message ID | 20200328073040.24429-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/ocrdma: Fix an off-by-one issue in 'ocrdma_add_stat' | expand |
On Sat, Mar 28, 2020 at 08:30:40AM +0100, Christophe JAILLET wrote: > There is an off-by-one issue when checking if there is enough space in the > output buffer, because we must keep some place for a final '\0'. > > While at it: > - Use 'scnprintf' instead of 'snprintf' in order to avoid a superfluous > 'strlen' > - avoid some useless initializations > - avoida hard coded buffer size that can be computed at built time. > > Fixes: a51f06e1679e ("RDMA/ocrdma: Query controller information") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > The '\0' comes from memset(..., 0, ...) in all callers. > This could be also avoided if needed. > --- > drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > index 5f831e3bdbad..614a449e6b87 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > @@ -49,13 +49,12 @@ static struct dentry *ocrdma_dbgfs_dir; > static int ocrdma_add_stat(char *start, char *pcur, > char *name, u64 count) > { > - char buff[128] = {0}; > - int cpy_len = 0; > + char buff[128]; > + int cpy_len; > > - snprintf(buff, 128, "%s: %llu\n", name, count); > - cpy_len = strlen(buff); > + cpy_len = scnprintf(buff, sizeof(buff), "%s: %llu\n", name, count); > > - if (pcur + cpy_len > start + OCRDMA_MAX_DBGFS_MEM) { > + if (pcur + cpy_len >= start + OCRDMA_MAX_DBGFS_MEM) { > pr_err("%s: No space in stats buff\n", __func__); > return 0; > } The memcpy is still kind of silly right? What about this: static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) { size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; int cpy_len; cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); if (cpy_len >= len || cpy_len < 0) { pr_err("%s: No space in stats buff\n", __func__); return 0; } return cpy_len; } Jason
On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > The memcpy is still kind of silly right? What about this: > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > { > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > int cpy_len; > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > if (cpy_len >= len || cpy_len < 0) { The kernel version of snprintf() doesn't and will never return negatives. It would cause a huge security headache if it started returning negatives. > pr_err("%s: No space in stats buff\n", __func__); > return 0; > } regards, dan carpenter
On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > The memcpy is still kind of silly right? What about this: > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > { > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > int cpy_len; > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > if (cpy_len >= len || cpy_len < 0) { > > The kernel version of snprintf() doesn't and will never return > negatives. It would cause a huge security headache if it started > returning negatives. Begs the question why it returns an int then :) Thanks, Jason
On Thu, Apr 16, 2020 at 03:47:54PM -0300, Jason Gunthorpe wrote: > On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > > The memcpy is still kind of silly right? What about this: > > > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > > { > > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > > int cpy_len; > > > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > > if (cpy_len >= len || cpy_len < 0) { > > > > The kernel version of snprintf() doesn't and will never return > > negatives. It would cause a huge security headache if it started > > returning negatives. > > Begs the question why it returns an int then :) People should use "int" as their default type. "int i;". It means "This is a normal number. Nothing special about it. It's not too high. It's not defined by hardware requirements." Other types call attention to themselves, but int is the humble datatype. regards, dan carpenter
On Fri, Apr 17, 2020 at 02:26:24PM +0300, Dan Carpenter wrote: > On Thu, Apr 16, 2020 at 03:47:54PM -0300, Jason Gunthorpe wrote: > > On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > > > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > > > The memcpy is still kind of silly right? What about this: > > > > > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > > > { > > > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > > > int cpy_len; > > > > > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > > > if (cpy_len >= len || cpy_len < 0) { > > > > > > The kernel version of snprintf() doesn't and will never return > > > negatives. It would cause a huge security headache if it started > > > returning negatives. > > > > Begs the question why it returns an int then :) > > People should use "int" as their default type. "int i;". It means > "This is a normal number. Nothing special about it. It's not too high. > It's not defined by hardware requirements." Other types call attention > to themselves, but int is the humble datatype. No, I strongly disagree with this, it is one of my pet peeves to see 'int' being used for data which is known to be only ever be positive just to save typing 'unsigned'. Not only is it confusing, but allowing signed values has caused tricky security bugs, unfortuntely. Jason
On Fri, Apr 17, 2020 at 09:25:42AM -0300, Jason Gunthorpe wrote: > On Fri, Apr 17, 2020 at 02:26:24PM +0300, Dan Carpenter wrote: > > On Thu, Apr 16, 2020 at 03:47:54PM -0300, Jason Gunthorpe wrote: > > > On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > > > > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > > > > The memcpy is still kind of silly right? What about this: > > > > > > > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > > > > { > > > > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > > > > int cpy_len; > > > > > > > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > > > > if (cpy_len >= len || cpy_len < 0) { > > > > > > > > The kernel version of snprintf() doesn't and will never return > > > > negatives. It would cause a huge security headache if it started > > > > returning negatives. > > > > > > Begs the question why it returns an int then :) > > > > People should use "int" as their default type. "int i;". It means > > "This is a normal number. Nothing special about it. It's not too high. > > It's not defined by hardware requirements." Other types call attention > > to themselves, but int is the humble datatype. > > No, I strongly disagree with this, it is one of my pet peeves to see > 'int' being used for data which is known to be only ever be positive > just to save typing 'unsigned'. > > Not only is it confusing, but allowing signed values has caused tricky > security bugs, unfortuntely. I have the opposite pet peeve. I complain about it a lot. It pains me every time I see a "u32 i;". I think there is a static analysis warning for using signed which encourages people to write code like that. That warning really upsets me for two reasons 1) The static checker should know the range of values but it doesn't so it makes me sad to see inferior technology being used when it should deleted instead. 2) I have never seen this warning prevent a real life bug. You would need to hit a series of fairly rare events for this warning to be useful and I have never seen that happen yet. The most common bug caused by unsigned variables is that it breaks the kernel error handling but there are other problems as well. There was an example a little while back where someone "fixed" a security problem by making things unsigned. for (i = 0; i < user_value; i++) { Originally if user_value was an int then the loop would have been a harmless no-op but now it was a large positive value so it lead to memory corruption. Another example is: for (i = 0; i < user_value - 1; i++) { If "user_value" is zero the subtraction becomes UINT_MAX. Or some people use a "u16 i;" but then the limit increases so the loop doesn't work any more. From my experience with static analysis and security audits, making things unsigned en mass causes more security bugs. There are definitely times where making variables unsigned is correct for security reasons like when you are taking a size from userspace. Complicated types call attention to themselves and they hurt readability. You sometimes *need* other datatypes and you want those to stand out but if everything is special then nothing is special. regards, dan carpenter
Le 14/04/2020 à 20:34, Jason Gunthorpe a écrit : > On Sat, Mar 28, 2020 at 08:30:40AM +0100, Christophe JAILLET wrote: >> There is an off-by-one issue when checking if there is enough space in the >> output buffer, because we must keep some place for a final '\0'. >> >> While at it: >> - Use 'scnprintf' instead of 'snprintf' in order to avoid a superfluous >> 'strlen' >> - avoid some useless initializations >> - avoida hard coded buffer size that can be computed at built time. >> >> Fixes: a51f06e1679e ("RDMA/ocrdma: Query controller information") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> The '\0' comes from memset(..., 0, ...) in all callers. >> This could be also avoided if needed. >> --- >> drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> index 5f831e3bdbad..614a449e6b87 100644 >> --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> @@ -49,13 +49,12 @@ static struct dentry *ocrdma_dbgfs_dir; >> static int ocrdma_add_stat(char *start, char *pcur, >> char *name, u64 count) >> { >> - char buff[128] = {0}; >> - int cpy_len = 0; >> + char buff[128]; >> + int cpy_len; >> >> - snprintf(buff, 128, "%s: %llu\n", name, count); >> - cpy_len = strlen(buff); >> + cpy_len = scnprintf(buff, sizeof(buff), "%s: %llu\n", name, count); >> >> - if (pcur + cpy_len > start + OCRDMA_MAX_DBGFS_MEM) { >> + if (pcur + cpy_len >= start + OCRDMA_MAX_DBGFS_MEM) { >> pr_err("%s: No space in stats buff\n", __func__); >> return 0; >> } > The memcpy is still kind of silly right? What about this: > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > { > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > int cpy_len; > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > if (cpy_len >= len || cpy_len < 0) { > pr_err("%s: No space in stats buff\n", __func__); > return 0; > } > return cpy_len; > } > > Jason It can looks useless, but I think that the goal was to make sure that we would not display truncated data. Each line is either complete or absent. I don't have any strong opinion of what is best, but I can understand the current logic. This function is not a hot spot, so useless memcpy is not a big issue. CJ
On Fri, Apr 17, 2020 at 04:09:55PM +0300, Dan Carpenter wrote: > On Fri, Apr 17, 2020 at 09:25:42AM -0300, Jason Gunthorpe wrote: > > On Fri, Apr 17, 2020 at 02:26:24PM +0300, Dan Carpenter wrote: > > > On Thu, Apr 16, 2020 at 03:47:54PM -0300, Jason Gunthorpe wrote: > > > > On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > > > > > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > > > > > The memcpy is still kind of silly right? What about this: > > > > > > > > > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > > > > > { > > > > > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > > > > > int cpy_len; > > > > > > > > > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > > > > > if (cpy_len >= len || cpy_len < 0) { > > > > > > > > > > The kernel version of snprintf() doesn't and will never return > > > > > negatives. It would cause a huge security headache if it started > > > > > returning negatives. > > > > > > > > Begs the question why it returns an int then :) > > > > > > People should use "int" as their default type. "int i;". It means > > > "This is a normal number. Nothing special about it. It's not too high. > > > It's not defined by hardware requirements." Other types call attention > > > to themselves, but int is the humble datatype. > > > > No, I strongly disagree with this, it is one of my pet peeves to see > > 'int' being used for data which is known to be only ever be positive > > just to save typing 'unsigned'. > > > > Not only is it confusing, but allowing signed values has caused tricky > > security bugs, unfortuntely. > > I have the opposite pet peeve. > > I complain about it a lot. It pains me every time I see a "u32 i;". I > think there is a static analysis warning for using signed which > encourages people to write code like that. That warning really upsets > me for two reasons 1) The static checker should know the range of values > but it doesn't so it makes me sad to see inferior technology being used > when it should deleted instead. 2) I have never seen this warning > prevent a real life bug. I have.. But I'm having trouble finding it in the git torrent.. Maybe this one: commit c2b37f76485f073f020e60b5954b6dc4e55f693c Author: Boris Pismenny <borisp@mellanox.com> Date: Thu Mar 8 15:51:41 2018 +0200 IB/mlx5: Fix integer overflows in mlx5_ib_create_srq > You would need to hit a series of fairly rare events for this > warning to be useful and I have never seen that happen yet. IIRC the case was the uapi rightly used u32, which was then wrongly implicitly cast to some internal function, accepting int, which then did something sort of like int len if (len >= sizeof(a)) return -EINVAL copy_from_user(a, b, len) Which explodes when a negative len is implicitly cast to unsigned long to call copy_from_user. > The most common bug caused by unsigned variables is that it breaks the > kernel error handling You mean returning -ERRNO? Sure, those should be int, but that is a case where a value actually can take on -ve numbers, so it really should be signed. > but there are other problems as well. There was an example a little > while back where someone "fixed" a security problem by making things > unsigned. > > for (i = 0; i < user_value; i++) { This is clearly missing input validation on user_value, the only reason int helps at all here is pure dumb luck for this one case. If it had used something like copy_to_user it would be broken. > Originally if user_value was an int then the loop would have been a > harmless no-op but now it was a large positive value so it lead to > memory corruption. Another example is: > > for (i = 0; i < user_value - 1; i++) { Again, code like this is simply missing required input validation. The for loop works with int by dumb luck, and this would be broken if it called copy_from_user. > From my experience with static analysis and security audits, making > things unsigned en mass causes more security bugs. There are definitely > times where making variables unsigned is correct for security reasons > like when you are taking a size from userspace. Any code that casts a unsigned value from userspace to a signed value in the kernel is deeply suspect, IMHO. If you get the in habit of using types properly then it is less likely this bug-class will happen. If your habit is to just always use 'int' for everything then you *will* accidently cause a user value to be implicitly casted. > Complicated types call attention to themselves and they hurt > readability. You sometimes *need* other datatypes and you want those to > stand out but if everything is special then nothing is special. If the programmer knows the value is never negative it should be recorded in the code, otherwise it is hard to tell if there are problems or not. Is this code wrong? int array_idx; ... if (array_idx < ARRAY_SIZE(foo)) return foo[array_idx]; Since 'int' was used the entire code flow has to be studied to determine if 'array_idx' is ever accidently set to negative. If it is unsigned I can tell you there is no problem right away. I do agree with you that people blindly changing things due to security scanners is not good.. Jason
On Fri, Apr 17, 2020 at 03:28:21PM +0200, Marion & Christophe JAILLET wrote: > > Le 14/04/2020 à 20:34, Jason Gunthorpe a écrit : > > On Sat, Mar 28, 2020 at 08:30:40AM +0100, Christophe JAILLET wrote: > > > There is an off-by-one issue when checking if there is enough space in the > > > output buffer, because we must keep some place for a final '\0'. > > > > > > While at it: > > > - Use 'scnprintf' instead of 'snprintf' in order to avoid a superfluous > > > 'strlen' > > > - avoid some useless initializations > > > - avoida hard coded buffer size that can be computed at built time. > > > > > > Fixes: a51f06e1679e ("RDMA/ocrdma: Query controller information") > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > The '\0' comes from memset(..., 0, ...) in all callers. > > > This could be also avoided if needed. > > > drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > > > index 5f831e3bdbad..614a449e6b87 100644 > > > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > > > @@ -49,13 +49,12 @@ static struct dentry *ocrdma_dbgfs_dir; > > > static int ocrdma_add_stat(char *start, char *pcur, > > > char *name, u64 count) > > > { > > > - char buff[128] = {0}; > > > - int cpy_len = 0; > > > + char buff[128]; > > > + int cpy_len; > > > - snprintf(buff, 128, "%s: %llu\n", name, count); > > > - cpy_len = strlen(buff); > > > + cpy_len = scnprintf(buff, sizeof(buff), "%s: %llu\n", name, count); > > > - if (pcur + cpy_len > start + OCRDMA_MAX_DBGFS_MEM) { > > > + if (pcur + cpy_len >= start + OCRDMA_MAX_DBGFS_MEM) { > > > pr_err("%s: No space in stats buff\n", __func__); > > > return 0; > > > } > > The memcpy is still kind of silly right? What about this: > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > { > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > int cpy_len; > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > if (cpy_len >= len || cpy_len < 0) { > > pr_err("%s: No space in stats buff\n", __func__); > > return 0; > > } > > return cpy_len; > > } > > > > Jason > > It can looks useless, but I think that the goal was to make sure that we > would not display truncated data. Each line is either complete or absent. So it needsa *pcur = 0 in the error path? Jason
Le 17/04/2020 à 15:50, Jason Gunthorpe a écrit : > On Fri, Apr 17, 2020 at 03:28:21PM +0200, Marion & Christophe JAILLET wrote: >> Le 14/04/2020 à 20:34, Jason Gunthorpe a écrit : >>> On Sat, Mar 28, 2020 at 08:30:40AM +0100, Christophe JAILLET wrote: >>>> There is an off-by-one issue when checking if there is enough space in the >>>> output buffer, because we must keep some place for a final '\0'. >>>> >>>> While at it: >>>> - Use 'scnprintf' instead of 'snprintf' in order to avoid a superfluous >>>> 'strlen' >>>> - avoid some useless initializations >>>> - avoida hard coded buffer size that can be computed at built time. >>>> >>>> Fixes: a51f06e1679e ("RDMA/ocrdma: Query controller information") >>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>> The '\0' comes from memset(..., 0, ...) in all callers. >>>> This could be also avoided if needed. >>>> drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 9 ++++----- >>>> 1 file changed, 4 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >>>> index 5f831e3bdbad..614a449e6b87 100644 >>>> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >>>> @@ -49,13 +49,12 @@ static struct dentry *ocrdma_dbgfs_dir; >>>> static int ocrdma_add_stat(char *start, char *pcur, >>>> char *name, u64 count) >>>> { >>>> - char buff[128] = {0}; >>>> - int cpy_len = 0; >>>> + char buff[128]; >>>> + int cpy_len; >>>> - snprintf(buff, 128, "%s: %llu\n", name, count); >>>> - cpy_len = strlen(buff); >>>> + cpy_len = scnprintf(buff, sizeof(buff), "%s: %llu\n", name, count); >>>> - if (pcur + cpy_len > start + OCRDMA_MAX_DBGFS_MEM) { >>>> + if (pcur + cpy_len >= start + OCRDMA_MAX_DBGFS_MEM) { >>>> pr_err("%s: No space in stats buff\n", __func__); >>>> return 0; >>>> } >>> The memcpy is still kind of silly right? What about this: >>> >>> static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) >>> { >>> size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; >>> int cpy_len; >>> >>> cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); >>> if (cpy_len >= len || cpy_len < 0) { >>> pr_err("%s: No space in stats buff\n", __func__); >>> return 0; >>> } >>> return cpy_len; >>> } >>> >>> Jason >> It can looks useless, but I think that the goal was to make sure that we >> would not display truncated data. Each line is either complete or absent. > So it needsa *pcur = 0 in the error path? > > Jason > I guess it would keep the existing behavior, should it be needed. I leave maintainers to choose what looks more readable to them, or just to ignore the patch if they think it is useless. Feel free to propose your version as a patch. Anyway, thanks for sharing alternative solutions. CJ
On Fri, Apr 17, 2020 at 10:48:16AM -0300, Jason Gunthorpe wrote: > On Fri, Apr 17, 2020 at 04:09:55PM +0300, Dan Carpenter wrote: > > On Fri, Apr 17, 2020 at 09:25:42AM -0300, Jason Gunthorpe wrote: > > > On Fri, Apr 17, 2020 at 02:26:24PM +0300, Dan Carpenter wrote: > > > > On Thu, Apr 16, 2020 at 03:47:54PM -0300, Jason Gunthorpe wrote: > > > > > On Thu, Apr 16, 2020 at 04:08:47PM +0300, Dan Carpenter wrote: > > > > > > On Tue, Apr 14, 2020 at 03:34:41PM -0300, Jason Gunthorpe wrote: > > > > > > > The memcpy is still kind of silly right? What about this: > > > > > > > > > > > > > > static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) > > > > > > > { > > > > > > > size_t len = (start + OCRDMA_MAX_DBGFS_MEM) - pcur; > > > > > > > int cpy_len; > > > > > > > > > > > > > > cpy_len = snprintf(pcur, len, "%s: %llu\n", name, count); > > > > > > > if (cpy_len >= len || cpy_len < 0) { > > > > > > > > > > > > The kernel version of snprintf() doesn't and will never return > > > > > > negatives. It would cause a huge security headache if it started > > > > > > returning negatives. > > > > > > > > > > Begs the question why it returns an int then :) > > > > > > > > People should use "int" as their default type. "int i;". It means > > > > "This is a normal number. Nothing special about it. It's not too high. > > > > It's not defined by hardware requirements." Other types call attention > > > > to themselves, but int is the humble datatype. > > > > > > No, I strongly disagree with this, it is one of my pet peeves to see > > > 'int' being used for data which is known to be only ever be positive > > > just to save typing 'unsigned'. > > > > > > Not only is it confusing, but allowing signed values has caused tricky > > > security bugs, unfortuntely. > > > > I have the opposite pet peeve. > > > > I complain about it a lot. It pains me every time I see a "u32 i;". I > > think there is a static analysis warning for using signed which > > encourages people to write code like that. That warning really upsets > > me for two reasons 1) The static checker should know the range of values > > but it doesn't so it makes me sad to see inferior technology being used > > when it should deleted instead. 2) I have never seen this warning > > prevent a real life bug. > > I have.. But I'm having trouble finding it in the git torrent.. > > Maybe this one: > > commit c2b37f76485f073f020e60b5954b6dc4e55f693c > Author: Boris Pismenny <borisp@mellanox.com> > Date: Thu Mar 8 15:51:41 2018 +0200 > > IB/mlx5: Fix integer overflows in mlx5_ib_create_srq > I was just meant unsigned iterators, not sizes. I consider that to be a different sort of bug. The original code did this: desc_size = max_t(int, 32, desc_size); Using signed casts for min_t() always seems like a crazy thing to me. I have a static checker warning for those but I think people didn't accept my patches for those if it was only for kernel hardenning and readability instead of to fix bugs. I don't know why, maybe casting to an int is faster? > > You would need to hit a series of fairly rare events for this > > warning to be useful and I have never seen that happen yet. > > IIRC the case was the uapi rightly used u32, which was then wrongly > implicitly cast to some internal function, accepting int, which then > did something sort of like > > int len > if (len >= sizeof(a)) > return -EINVAL > copy_from_user(a, b, len) This code works. "len" is type promoted to unsigned and negative values are rejected. > > Which explodes when a negative len is implicitly cast to unsigned long > to call copy_from_user. > > > The most common bug caused by unsigned variables is that it breaks the > > kernel error handling > > You mean returning -ERRNO? Sure, those should be int, but that is a > case where a value actually can take on -ve numbers, so it really > should be signed. > > > but there are other problems as well. There was an example a little > > while back where someone "fixed" a security problem by making things > > unsigned. > > > > for (i = 0; i < user_value; i++) { > > This is clearly missing input validation on user_value, the only > reason int helps at all here is pure dumb luck for this one case. > > If it had used something like copy_to_user it would be broken. The real life example was slightly more complicated than that. But the point is that a lot of people think unsigned values are inherently more safe and they use u32 everywhere as a default datatype. I argue that the default should always be int unless there is a good reason otherwise. In my own Smatch code, I have a u16 struct member which constantly causes me bugs. But I keep it because the struct is carefully aligned to save memory. There are reasons for the other datatypes to exist, but using them is tricky so it's best to avoid it if you can. There is a lot of magic to making your limits unsigned long type. > > > Originally if user_value was an int then the loop would have been a > > harmless no-op but now it was a large positive value so it lead to > > memory corruption. Another example is: > > > > for (i = 0; i < user_value - 1; i++) { > > Again, code like this is simply missing required input validation. The > for loop works with int by dumb luck, and this would be broken if it > called copy_from_user. The thing about int type is that it works like people expect normal numbers to work. People normally think that zero minus one is going to be negative but if they change to u32 by default then it wraps to UINT_MAX and that's unexpected. There is an element where the static checker encourages people to "change your types to match" and that's garbage advice. Changing your types doesn't magically make things better and I would argue that it normally makes things worse. > > > From my experience with static analysis and security audits, making > > things unsigned en mass causes more security bugs. There are definitely > > times where making variables unsigned is correct for security reasons > > like when you are taking a size from userspace. > > Any code that casts a unsigned value from userspace to a signed value > in the kernel is deeply suspect, IMHO. Agreed. > > If you get the in habit of using types properly then it is less likely > this bug-class will happen. If your habit is to just always use 'int' > for everything then you *will* accidently cause a user value to be > implicitly casted. This is an interesting theory but I haven't seen any evidence to support it. My intuition is that it's better to only care when you have to otherwise you get overwhelmed. > > > Complicated types call attention to themselves and they hurt > > readability. You sometimes *need* other datatypes and you want those to > > stand out but if everything is special then nothing is special. > > If the programmer knows the value is never negative it should be > recorded in the code, otherwise it is hard to tell if there are > problems or not. > > Is this code wrong? > > int array_idx; > ... > if (array_idx < ARRAY_SIZE(foo)) > return foo[array_idx]; In some ways, I'm the wrong person to ask because I know without even thinking about it that ARRAY_SIZE() is size_t so the code works fine... regards, dan carpenter
On Fri, Apr 17, 2020 at 06:13:14PM +0300, Dan Carpenter wrote: > I was just meant unsigned iterators, not sizes. I consider that to be a > different sort of bug. The original code did this: > > desc_size = max_t(int, 32, desc_size); > > Using signed casts for min_t() always seems like a crazy thing to me. I > have a static checker warning for those but I think people didn't accept > my patches for those if it was only for kernel hardenning and > readability instead of to fix bugs. I don't know why, maybe casting to > an int is faster? Casting usually doesn't cost anything... But I think this shows again why int is trouble: most likely desc_size is a unsigned value stored in an int, and the temptation of max_t is to use the type of the output variable. So 'int' is a logical, if not bonkers choice. If desc_size was properly unsigned then the author should keep using unsigned through the max_t() > > > You would need to hit a series of fairly rare events for this > > > warning to be useful and I have never seen that happen yet. > > > > IIRC the case was the uapi rightly used u32, which was then wrongly > > implicitly cast to some internal function, accepting int, which then > > did something sort of like > > > > int len > > if (len >= sizeof(a)) > > return -EINVAL > > copy_from_user(a, b, len) > > This code works. "len" is type promoted to unsigned and negative values > are rejected. Expecting people to know the unsigned/sign implicit promotion rules for expressions to determine the code is right/wrong is just asking to much, IMHO. I certainly don't have them all memorized and try to avoid them :( Using int pretty much guarentees you hit those cases... > The real life example was slightly more complicated than that. But the > point is that a lot of people think unsigned values are inherently more > safe and they use u32 everywhere as a default datatype. I argue that > the default should always be int unless there is a good reason > otherwise. Why? In my experience most values actually are never negative. > to save memory. There are reasons for the other datatypes to exist, but > using them is tricky so it's best to avoid it if you can. I can't say I have the same experience.. > There is a lot of magic to making your limits unsigned long type. Oh? More magic than int is implictly promoted to unsigned anyhow? > > > Originally if user_value was an int then the loop would have been a > > > harmless no-op but now it was a large positive value so it lead to > > > memory corruption. Another example is: > > > > > > for (i = 0; i < user_value - 1; i++) { > > > > Again, code like this is simply missing required input validation. The > > for loop works with int by dumb luck, and this would be broken if it > > called copy_from_user. > > The thing about int type is that it works like people expect normal > numbers to work. Not really. Some cases are a bit better, some are worse. As above when using int: -1 >= sizeof(A) == true Which is not at all how any sane person thinks about normal numbers. There are lots of these odd traps around implicit promotion. While foo-1 is right there, explicitly. If foo-1 < 0 and the code is not expecting it then you have a clear problem. > People normally think that zero minus one is going to > be negative but if they change to u32 by default then it wraps to > UINT_MAX and that's unexpected. Maybe I've been doing this too long, but this seems totally expected to me... > There is an element where the static checker encourages people to > "change your types to match" and that's garbage advice. Changing > your types doesn't magically make things better and I would argue > that it normally makes things worse. I don't know much about this warning, but I do find that people starting out tend to just use 'int' everywhere and 'hope for the best' without any clear understanding or thinking of what types are what. > > If you get the in habit of using types properly then it is less likely > > this bug-class will happen. If your habit is to just always use 'int' > > for everything then you *will* accidently cause a user value to be > > implicitly casted. > > This is an interesting theory but I haven't seen any evidence to support > it. My intuition is that it's better to only care when you have to > otherwise you get overwhelmed. If you make everything unsigned, there really is no downside, other than possible subtraction related issues that exist anyhow, AFAIK. This is the approach the C std uses, pretty much the entire API is properly marked with signed/unsigned. I feel in pretty good company advocating that this is the best way to write C code :) But then again, I find the modular math intuitive and aware to be careful with subtraction... Jason
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c index 5f831e3bdbad..614a449e6b87 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c @@ -49,13 +49,12 @@ static struct dentry *ocrdma_dbgfs_dir; static int ocrdma_add_stat(char *start, char *pcur, char *name, u64 count) { - char buff[128] = {0}; - int cpy_len = 0; + char buff[128]; + int cpy_len; - snprintf(buff, 128, "%s: %llu\n", name, count); - cpy_len = strlen(buff); + cpy_len = scnprintf(buff, sizeof(buff), "%s: %llu\n", name, count); - if (pcur + cpy_len > start + OCRDMA_MAX_DBGFS_MEM) { + if (pcur + cpy_len >= start + OCRDMA_MAX_DBGFS_MEM) { pr_err("%s: No space in stats buff\n", __func__); return 0; }
There is an off-by-one issue when checking if there is enough space in the output buffer, because we must keep some place for a final '\0'. While at it: - Use 'scnprintf' instead of 'snprintf' in order to avoid a superfluous 'strlen' - avoid some useless initializations - avoida hard coded buffer size that can be computed at built time. Fixes: a51f06e1679e ("RDMA/ocrdma: Query controller information") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- The '\0' comes from memset(..., 0, ...) in all callers. This could be also avoided if needed. --- drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)