| Message ID | ab56c444-418a-423d-8528-cf04d5d458ef@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: stk1160: fix some bounds checking in stk1160_copy_video() | expand |
Hi Dan On Wed, 17 Apr 2024 at 19:51, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The subtract in this condition is reversed. The ->length is the length > of the buffer. The ->bytesused is how many bytes we have copied thus > far. When the condition is reversed that means the result of the > subtraction is always negative but since it's unsigned then the result > is a very high positive value. That means the overflow check is never > true. > > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > This patch is untested, I just spotted it in review. > > When this bug is fixed, the two checks for negative values of "lencopy" > could be removed. I wrote a version of this patch which removed the > checks, but in the end I decided to leave the checks. They're harmless. > > drivers/media/usb/stk1160/stk1160-video.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c > index 366f0e4a5dc0..bfb97ea352e7 100644 > --- a/drivers/media/usb/stk1160/stk1160-video.c > +++ b/drivers/media/usb/stk1160/stk1160-video.c > @@ -139,8 +139,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) > * Check if we have enough space left in the buffer. > * In that case, we force loop exit after copy. > */ > - if (lencopy > buf->bytesused - buf->length) { > - lencopy = buf->bytesused - buf->length; > + if (lencopy > buf->length - buf->bytesused) { > + lencopy = buf->length - buf->bytesused; > remain = lencopy; > } I think it is a bit more complicated than bytesused. bytesused does not take into consideration the actual position where it is going to write. What we really want to check is if offset = dst - buf->mem; if (offset + lencopy > buf->length) { lencopy = buf->length - offset; remain = lencopy; } > > @@ -182,8 +182,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) > * Check if we have enough space left in the buffer. > * In that case, we force loop exit after copy. > */ > - if (lencopy > buf->bytesused - buf->length) { > - lencopy = buf->bytesused - buf->length; > + if (lencopy > buf->length - buf->bytesused) { > + lencopy = buf->length - buf->bytesused; > remain = lencopy; > } > > -- > 2.43.0 >
On Wed, Apr 17, 2024 at 08:48:23PM +0200, Ricardo Ribalda wrote: > Hi Dan > > On Wed, 17 Apr 2024 at 19:51, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > The subtract in this condition is reversed. The ->length is the length > > of the buffer. The ->bytesused is how many bytes we have copied thus > > far. When the condition is reversed that means the result of the > > subtraction is always negative but since it's unsigned then the result > > is a very high positive value. That means the overflow check is never > > true. > > > > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > This patch is untested, I just spotted it in review. > > > > When this bug is fixed, the two checks for negative values of "lencopy" > > could be removed. I wrote a version of this patch which removed the > > checks, but in the end I decided to leave the checks. They're harmless. > > > > drivers/media/usb/stk1160/stk1160-video.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c > > index 366f0e4a5dc0..bfb97ea352e7 100644 > > --- a/drivers/media/usb/stk1160/stk1160-video.c > > +++ b/drivers/media/usb/stk1160/stk1160-video.c > > @@ -139,8 +139,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) > > * Check if we have enough space left in the buffer. > > * In that case, we force loop exit after copy. > > */ > > - if (lencopy > buf->bytesused - buf->length) { > > - lencopy = buf->bytesused - buf->length; > > + if (lencopy > buf->length - buf->bytesused) { > > + lencopy = buf->length - buf->bytesused; > > remain = lencopy; > > } > > I think it is a bit more complicated than bytesused. > > bytesused does not take into consideration the actual position where > it is going to write. > > What we really want to check is if > > offset = dst - buf->mem; > if (offset + lencopy > buf->length) { > lencopy = buf->length - offset; > remain = lencopy; > } > You're right... There is a comment explaining why we multiply the number of lines written by two, but it doesn't really clarify anything for me: /* Multiply linesdone by two, to take account of the other field */ What's the "other field"? I kind of suspect that the stk1160_buffer_done() might be wrong as well. vb2_set_plane_payload(&buf->vb.vb2_buf, 0, buf->bytesused); ^^^^^^^^^^^^^^ We're calculating the space left based on ->pos which can be reset to zero in stk1160_process_isoc(). But ->bytesused isn't reset, so potentially we could end up in a situation where ->bytesused is greater than the ->length of the buffer. Should stk1160_process_isoc() set ->bytesused to zero as well? regards, dan carpenter
Hi Dan On Mon, 22 Apr 2024 at 17:30, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Wed, Apr 17, 2024 at 08:48:23PM +0200, Ricardo Ribalda wrote: > > Hi Dan > > > > On Wed, 17 Apr 2024 at 19:51, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > > > The subtract in this condition is reversed. The ->length is the length > > > of the buffer. The ->bytesused is how many bytes we have copied thus > > > far. When the condition is reversed that means the result of the > > > subtraction is always negative but since it's unsigned then the result > > > is a very high positive value. That means the overflow check is never > > > true. > > > > > > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > This patch is untested, I just spotted it in review. > > > > > > When this bug is fixed, the two checks for negative values of "lencopy" > > > could be removed. I wrote a version of this patch which removed the > > > checks, but in the end I decided to leave the checks. They're harmless. > > > > > > drivers/media/usb/stk1160/stk1160-video.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c > > > index 366f0e4a5dc0..bfb97ea352e7 100644 > > > --- a/drivers/media/usb/stk1160/stk1160-video.c > > > +++ b/drivers/media/usb/stk1160/stk1160-video.c > > > @@ -139,8 +139,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) > > > * Check if we have enough space left in the buffer. > > > * In that case, we force loop exit after copy. > > > */ > > > - if (lencopy > buf->bytesused - buf->length) { > > > - lencopy = buf->bytesused - buf->length; > > > + if (lencopy > buf->length - buf->bytesused) { > > > + lencopy = buf->length - buf->bytesused; > > > remain = lencopy; > > > } > > > > I think it is a bit more complicated than bytesused. > > > > bytesused does not take into consideration the actual position where > > it is going to write. > > > > What we really want to check is if > > > > offset = dst - buf->mem; > > if (offset + lencopy > buf->length) { > > lencopy = buf->length - offset; > > remain = lencopy; > > } > > > > You're right... There is a comment explaining why we multiply the > number of lines written by two, but it doesn't really clarify anything > for me: > > /* Multiply linesdone by two, to take account of the other field */ > > What's the "other field"? I guess it p[0]. Looks like the device sends first the data for the even lines, and then the data for the odd lines (or the other way around). And we are descrambling the data in the kernel The code uses p[0] to figure out if it is the beginning of a block of odds/evens, and if it is odd or even if (p[0] == 0xc0 || p[0] == 0x80) { /* We set next packet parity and * continue to get next one */ dev->isoc_ctl.buf->odd = *p & 0x40; dev->isoc_ctl.buf->pos = 0; continue; } > > I kind of suspect that the stk1160_buffer_done() might be wrong as well. > > vb2_set_plane_payload(&buf->vb.vb2_buf, 0, buf->bytesused); > ^^^^^^^^^^^^^^ > > We're calculating the space left based on ->pos which can be reset to > zero in stk1160_process_isoc(). But ->bytesused isn't reset, so > potentially we could end up in a situation where ->bytesused is greater > than the ->length of the buffer. Should stk1160_process_isoc() set > ->bytesused to zero as well? I do not think so. bytes->used is len(odd) + len(even). If you reset bytesused then you are only returning 1/2 the size > > regards, > dan carpenter >
diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c index 366f0e4a5dc0..bfb97ea352e7 100644 --- a/drivers/media/usb/stk1160/stk1160-video.c +++ b/drivers/media/usb/stk1160/stk1160-video.c @@ -139,8 +139,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) * Check if we have enough space left in the buffer. * In that case, we force loop exit after copy. */ - if (lencopy > buf->bytesused - buf->length) { - lencopy = buf->bytesused - buf->length; + if (lencopy > buf->length - buf->bytesused) { + lencopy = buf->length - buf->bytesused; remain = lencopy; } @@ -182,8 +182,8 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) * Check if we have enough space left in the buffer. * In that case, we force loop exit after copy. */ - if (lencopy > buf->bytesused - buf->length) { - lencopy = buf->bytesused - buf->length; + if (lencopy > buf->length - buf->bytesused) { + lencopy = buf->length - buf->bytesused; remain = lencopy; }
The subtract in this condition is reversed. The ->length is the length of the buffer. The ->bytesused is how many bytes we have copied thus far. When the condition is reversed that means the result of the subtraction is always negative but since it's unsigned then the result is a very high positive value. That means the overflow check is never true. Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- This patch is untested, I just spotted it in review. When this bug is fixed, the two checks for negative values of "lencopy" could be removed. I wrote a version of this patch which removed the checks, but in the end I decided to leave the checks. They're harmless. drivers/media/usb/stk1160/stk1160-video.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)