| Message ID | 8dc35a68-7257-41ac-9057-7c89b9ad6e18@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
On 6/21/2023 12:51 PM, Dan Carpenter wrote: > There are several issues in this code. The check at the start of the > loop: > > if (user_len >= user_msg->len) { > > This check does not ensure that we have enough space for the trans_hdr > (8 bytes). Instead the check needs to be: > > if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > > That subtraction is done as an unsigned long we want to avoid > negatives. Add a lower bound to the start of the function. > > if (user_msg->len < sizeof(*trans_hdr)) > > There is a second integer underflow which can happen if > trans_hdr->len is zero inside the encode_passthrough() function. > > memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr)); > > Instead of adding a check to encode_passthrough() it's better to check > in this central place. Add that check: > > if (trans_hdr->len < sizeof(trans_hdr) > > The final concern is that the "user_len + trans_hdr->len" might have an > integer overflow bug. Use size_add() to prevent that. > > - if (user_len + trans_hdr->len > user_msg->len) { > + if (size_add(user_len, trans_hdr->len) > user_msg->len) { > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > This is based on code review and not tested. > > drivers/accel/qaic/qaic_control.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > index 5c57f7b4494e..a51b1594dcfa 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > int ret; > int i; > > - if (!user_msg->count) { > + if (!user_msg->count || > + user_msg->len < sizeof(*trans_hdr)) { Can we have something like this here user_msg->len < sizeof(*trans_hdr) * user_msg->count, no? > ret = -EINVAL; > goto out; > } > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > } > > for (i = 0; i < user_msg->count; ++i) { > - if (user_len >= user_msg->len) { > + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { Do you think it is more readable if we have something like this user_len + sizeof(*trans_hdr) >= user_msg->len > ret = -EINVAL; > break; > } > trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len); > - if (user_len + trans_hdr->len > user_msg->len) { > + if (trans_hdr->len < sizeof(trans_hdr) || > + size_add(user_len, trans_hdr->len) > user_msg->len) { > ret = -EINVAL; > break; > } Hey Dan, Thank you for going through qaic driver. You patches are very much appreciated. This is good work.
On Thu, Jun 22, 2023 at 04:54:03PM +0530, Pranjal Ramajor Asha Kanojiya wrote: > > > On 6/21/2023 12:51 PM, Dan Carpenter wrote: > > There are several issues in this code. The check at the start of the > > loop: > > > > if (user_len >= user_msg->len) { > > > > This check does not ensure that we have enough space for the trans_hdr > > (8 bytes). Instead the check needs to be: > > > > if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > > > > That subtraction is done as an unsigned long we want to avoid > > negatives. Add a lower bound to the start of the function. > > > > if (user_msg->len < sizeof(*trans_hdr)) > > > > There is a second integer underflow which can happen if > > trans_hdr->len is zero inside the encode_passthrough() function. > > > > memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr)); > > > > Instead of adding a check to encode_passthrough() it's better to check > > in this central place. Add that check: > > > > if (trans_hdr->len < sizeof(trans_hdr) > > > > The final concern is that the "user_len + trans_hdr->len" might have an > > integer overflow bug. Use size_add() to prevent that. > > > > - if (user_len + trans_hdr->len > user_msg->len) { > > + if (size_add(user_len, trans_hdr->len) > user_msg->len) { > > > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > This is based on code review and not tested. > > > > drivers/accel/qaic/qaic_control.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > > index 5c57f7b4494e..a51b1594dcfa 100644 > > --- a/drivers/accel/qaic/qaic_control.c > > +++ b/drivers/accel/qaic/qaic_control.c > > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > int ret; > > int i; > > - if (!user_msg->count) { > > + if (!user_msg->count || > > + user_msg->len < sizeof(*trans_hdr)) { > Can we have something like this here > user_msg->len < sizeof(*trans_hdr) * user_msg->count, no? This check was just to ensure that we have space for one header so that the "user_msg->len - sizeof(*trans_hdr)" subtraction doesn't overflow. We're going to need to check that we have space for each header later anyway. Can the multiply fail (on 32bit)? > > ret = -EINVAL; > > goto out; > > } > > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > } > > for (i = 0; i < user_msg->count; ++i) { > > - if (user_len >= user_msg->len) { > > + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > Do you think it is more readable if we have something like this > user_len + sizeof(*trans_hdr) >= user_msg->len Either way works. The math should be on trusted side, and to me the form is always if (variable >= trusted value) { so I prefer to put the math on right. But here both sides are trusted and there is no risk of integer overflow. If we did that then we could remove the if (user_msg->len < sizeof(*trans_hdr)) condition from the start. regards, dan carpenter
On Thu, Jun 22, 2023 at 02:43:57PM +0300, Dan Carpenter wrote: > > > - if (!user_msg->count) { > > > + if (!user_msg->count || > > > + user_msg->len < sizeof(*trans_hdr)) { > > Can we have something like this here > > user_msg->len < sizeof(*trans_hdr) * user_msg->count, no? > > This check was just to ensure that we have space for one header so that > the "user_msg->len - sizeof(*trans_hdr)" subtraction doesn't overflow. > We're going to need to check that we have space for each header later > anyway. Can the multiply fail (on 32bit)? s/fail/integer overflow/. Obviously failure is not an option when it comes to multiplies. regards, dan carpenter
On 6/21/2023 12:51 PM, Dan Carpenter wrote: > There are several issues in this code. The check at the start of the > loop: > > if (user_len >= user_msg->len) { > > This check does not ensure that we have enough space for the trans_hdr > (8 bytes). Instead the check needs to be: > > if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > > That subtraction is done as an unsigned long we want to avoid > negatives. Add a lower bound to the start of the function. > > if (user_msg->len < sizeof(*trans_hdr)) > > There is a second integer underflow which can happen if > trans_hdr->len is zero inside the encode_passthrough() function. > > memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr)); > > Instead of adding a check to encode_passthrough() it's better to check > in this central place. Add that check: > > if (trans_hdr->len < sizeof(trans_hdr) > > The final concern is that the "user_len + trans_hdr->len" might have an > integer overflow bug. Use size_add() to prevent that. > > - if (user_len + trans_hdr->len > user_msg->len) { > + if (size_add(user_len, trans_hdr->len) > user_msg->len) { > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > This is based on code review and not tested. > > drivers/accel/qaic/qaic_control.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > index 5c57f7b4494e..a51b1594dcfa 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > int ret; > int i; > > - if (!user_msg->count) { > + if (!user_msg->count || > + user_msg->len < sizeof(*trans_hdr)) { > ret = -EINVAL; > goto out; > } > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > } > > for (i = 0; i < user_msg->count; ++i) { > - if (user_len >= user_msg->len) { > + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { If I understand correctly this check is added to verify if we are left with trans_hdr size of data. In that case '>' comparison operator should be used. > ret = -EINVAL; > break; > } > trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len); > - if (user_len + trans_hdr->len > user_msg->len) { > + if (trans_hdr->len < sizeof(trans_hdr) || > + size_add(user_len, trans_hdr->len) > user_msg->len) { > ret = -EINVAL; > break; > }
On 7/4/2023 11:57 AM, Pranjal Ramajor Asha Kanojiya wrote: > > > On 6/21/2023 12:51 PM, Dan Carpenter wrote: >> There are several issues in this code. The check at the start of the >> loop: >> >> if (user_len >= user_msg->len) { >> >> This check does not ensure that we have enough space for the trans_hdr >> (8 bytes). Instead the check needs to be: >> >> if (user_len >= user_msg->len - sizeof(*trans_hdr)) { >> >> That subtraction is done as an unsigned long we want to avoid >> negatives. Add a lower bound to the start of the function. >> >> if (user_msg->len < sizeof(*trans_hdr)) >> >> There is a second integer underflow which can happen if >> trans_hdr->len is zero inside the encode_passthrough() function. >> >> memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - >> sizeof(in_trans->hdr)); >> >> Instead of adding a check to encode_passthrough() it's better to check >> in this central place. Add that check: >> >> if (trans_hdr->len < sizeof(trans_hdr) >> >> The final concern is that the "user_len + trans_hdr->len" might have an >> integer overflow bug. Use size_add() to prevent that. >> >> - if (user_len + trans_hdr->len > user_msg->len) { >> + if (size_add(user_len, trans_hdr->len) > user_msg->len) { >> >> Fixes: 129776ac2e38 ("accel/qaic: Add control path") >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> This is based on code review and not tested. >> >> drivers/accel/qaic/qaic_control.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/accel/qaic/qaic_control.c >> b/drivers/accel/qaic/qaic_control.c >> index 5c57f7b4494e..a51b1594dcfa 100644 >> --- a/drivers/accel/qaic/qaic_control.c >> +++ b/drivers/accel/qaic/qaic_control.c >> @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device >> *qdev, struct manage_msg *user_msg, >> int ret; >> int i; >> - if (!user_msg->count) { >> + if (!user_msg->count || >> + user_msg->len < sizeof(*trans_hdr)) { >> ret = -EINVAL; >> goto out; >> } >> @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device >> *qdev, struct manage_msg *user_msg, >> } >> for (i = 0; i < user_msg->count; ++i) { >> - if (user_len >= user_msg->len) { >> + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > If I understand correctly this check is added to verify if we are left > with trans_hdr size of data. In that case '>' comparison operator should > be used. > >> ret = -EINVAL; >> break; >> } >> trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data >> + user_len); >> - if (user_len + trans_hdr->len > user_msg->len) { >> + if (trans_hdr->len < sizeof(trans_hdr) || >> + size_add(user_len, trans_hdr->len) > user_msg->len) { Since the size of characters per line is 100 now. Can we rearrange this if condition and have them in one line. Similarity at other places in this patch series. Thank you. >> ret = -EINVAL; >> break; >> }
On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote: > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > > index 5c57f7b4494e..a51b1594dcfa 100644 > > --- a/drivers/accel/qaic/qaic_control.c > > +++ b/drivers/accel/qaic/qaic_control.c > > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > int ret; > > int i; > > - if (!user_msg->count) { > > + if (!user_msg->count || > > + user_msg->len < sizeof(*trans_hdr)) { > > ret = -EINVAL; > > goto out; > > } > > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > } > > for (i = 0; i < user_msg->count; ++i) { > > - if (user_len >= user_msg->len) { > > + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > If I understand correctly this check is added to verify if we are left with > trans_hdr size of data. In that case '>' comparison operator should be used. That was there in the original code and I thought about changing it but I don't like changing things which aren't necessary and == is also invalid so I decided to leave it. > > > ret = -EINVAL; > > break; > > } > > trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len); > > - if (user_len + trans_hdr->len > user_msg->len) { > > + if (trans_hdr->len < sizeof(trans_hdr) || > > + size_add(user_len, trans_hdr->len) > user_msg->len) { If we change to > then the == will be caught by this check. So it doesn't affect runtime either way. regards, dan carpenter
On Tue, Jul 04, 2023 at 12:04:01PM +0530, Pranjal Ramajor Asha Kanojiya wrote: > > > -??????? if (user_len + trans_hdr->len > user_msg->len) { > > > +??????? if (trans_hdr->len < sizeof(trans_hdr) || > > > +??????????? size_add(user_len, trans_hdr->len) > user_msg->len) { > Since the size of characters per line is 100 now. Can we rearrange this if > condition and have them in one line. Similarity at other places in this > patch series. Style is subjective so I can't say for sure that my style is better but obviously it is. ;) Those are two separate conditions so I put them on two lines. If it were something very related like if (x < 0 || x >= 10) then I would have put it on one line. regards, dan carpenter
On 7/4/2023 2:08 PM, Dan Carpenter wrote: > On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote: >>> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c >>> index 5c57f7b4494e..a51b1594dcfa 100644 >>> --- a/drivers/accel/qaic/qaic_control.c >>> +++ b/drivers/accel/qaic/qaic_control.c >>> @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, >>> int ret; >>> int i; >>> - if (!user_msg->count) { >>> + if (!user_msg->count || >>> + user_msg->len < sizeof(*trans_hdr)) { >>> ret = -EINVAL; >>> goto out; >>> } >>> @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, >>> } >>> for (i = 0; i < user_msg->count; ++i) { >>> - if (user_len >= user_msg->len) { >>> + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { >> If I understand correctly this check is added to verify if we are left with >> trans_hdr size of data. In that case '>' comparison operator should be used. > > That was there in the original code and I thought about changing it but > I don't like changing things which aren't necessary and == is also > invalid so I decided to leave it. > I see, I understand your concern about not changing unnecessary things but '>=' is incorrect for reason mentioned above. We need to change that to '>' >> >>> ret = -EINVAL; >>> break; >>> } >>> trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len); >>> - if (user_len + trans_hdr->len > user_msg->len) { >>> + if (trans_hdr->len < sizeof(trans_hdr) || >>> + size_add(user_len, trans_hdr->len) > user_msg->len) { > > If we change to > then the == will be caught by this check. So it > doesn't affect runtime either way. > I fail to see that. Lets run an example: user_len is 0 user_msg->len is 8 sizeof(*trans_hdr) is 8 trans_hdr->len is 8 Above instance is correct and should be processed without error. So user_len > user_msg->len - sizeof(*trans_hdr) translates to (0 > 8 - 8) (0 > 0) false (No error) . . . trans_hdr->len < sizeof(trans_hdr) || size_add(user_len, trans_hdr->len) > user_msg->len, translates to 8 < 8 || size_add(0, 8) > 8 false || 8 > 8 false || false false (No error) Am I missing anything? > regards, > dan carpenter >
On Tue, Jul 04, 2023 at 03:18:26PM +0530, Pranjal Ramajor Asha Kanojiya wrote: > > > On 7/4/2023 2:08 PM, Dan Carpenter wrote: > > On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote: > > > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > > > > index 5c57f7b4494e..a51b1594dcfa 100644 > > > > --- a/drivers/accel/qaic/qaic_control.c > > > > +++ b/drivers/accel/qaic/qaic_control.c > > > > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > > > int ret; > > > > int i; > > > > - if (!user_msg->count) { > > > > + if (!user_msg->count || > > > > + user_msg->len < sizeof(*trans_hdr)) { > > > > ret = -EINVAL; > > > > goto out; > > > > } > > > > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, > > > > } > > > > for (i = 0; i < user_msg->count; ++i) { > > > > - if (user_len >= user_msg->len) { > > > > + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { > > > If I understand correctly this check is added to verify if we are left with > > > trans_hdr size of data. In that case '>' comparison operator should be used. > > > > That was there in the original code and I thought about changing it but > > I don't like changing things which aren't necessary and == is also > > invalid so I decided to leave it. > > > I see, I understand your concern about not changing unnecessary things but > '>=' is incorrect for reason mentioned above. We need to change that to '>' Oh, yes. You're right. I will need to resend. regards, dan carpenter
On 7/4/2023 3:58 AM, Dan Carpenter wrote: > On Tue, Jul 04, 2023 at 03:18:26PM +0530, Pranjal Ramajor Asha Kanojiya wrote: >> >> >> On 7/4/2023 2:08 PM, Dan Carpenter wrote: >>> On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote: >>>>> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c >>>>> index 5c57f7b4494e..a51b1594dcfa 100644 >>>>> --- a/drivers/accel/qaic/qaic_control.c >>>>> +++ b/drivers/accel/qaic/qaic_control.c >>>>> @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, >>>>> int ret; >>>>> int i; >>>>> - if (!user_msg->count) { >>>>> + if (!user_msg->count || >>>>> + user_msg->len < sizeof(*trans_hdr)) { >>>>> ret = -EINVAL; >>>>> goto out; >>>>> } >>>>> @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, >>>>> } >>>>> for (i = 0; i < user_msg->count; ++i) { >>>>> - if (user_len >= user_msg->len) { >>>>> + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { >>>> If I understand correctly this check is added to verify if we are left with >>>> trans_hdr size of data. In that case '>' comparison operator should be used. >>> >>> That was there in the original code and I thought about changing it but >>> I don't like changing things which aren't necessary and == is also >>> invalid so I decided to leave it. >>> >> I see, I understand your concern about not changing unnecessary things but >> '>=' is incorrect for reason mentioned above. We need to change that to '>' > > Oh, yes. You're right. I will need to resend. For the next revision, please add #include <overflow.h> I believe the size_add() use that you propose is the first need of that file, and while it may be implicitly included from something we do include, I prefer to have explicit includes. Otherwise I don't see anything else to add.
diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c index 5c57f7b4494e..a51b1594dcfa 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, int ret; int i; - if (!user_msg->count) { + if (!user_msg->count || + user_msg->len < sizeof(*trans_hdr)) { ret = -EINVAL; goto out; } @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg, } for (i = 0; i < user_msg->count; ++i) { - if (user_len >= user_msg->len) { + if (user_len >= user_msg->len - sizeof(*trans_hdr)) { ret = -EINVAL; break; } trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len); - if (user_len + trans_hdr->len > user_msg->len) { + if (trans_hdr->len < sizeof(trans_hdr) || + size_add(user_len, trans_hdr->len) > user_msg->len) { ret = -EINVAL; break; }
There are several issues in this code. The check at the start of the loop: if (user_len >= user_msg->len) { This check does not ensure that we have enough space for the trans_hdr (8 bytes). Instead the check needs to be: if (user_len >= user_msg->len - sizeof(*trans_hdr)) { That subtraction is done as an unsigned long we want to avoid negatives. Add a lower bound to the start of the function. if (user_msg->len < sizeof(*trans_hdr)) There is a second integer underflow which can happen if trans_hdr->len is zero inside the encode_passthrough() function. memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr)); Instead of adding a check to encode_passthrough() it's better to check in this central place. Add that check: if (trans_hdr->len < sizeof(trans_hdr) The final concern is that the "user_len + trans_hdr->len" might have an integer overflow bug. Use size_add() to prevent that. - if (user_len + trans_hdr->len > user_msg->len) { + if (size_add(user_len, trans_hdr->len) > user_msg->len) { Fixes: 129776ac2e38 ("accel/qaic: Add control path") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- This is based on code review and not tested. drivers/accel/qaic/qaic_control.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)