| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AM6PR04MB6341451D5139D2EFFDAD44DFE70B9@AM6PR04MB6341.eurprd04.prod.outlook.com>
Date: Thu, 10 Mar 2022 01:55:33 +0000
From: Ming Qian <ming.qian@....com>
To: Dan Carpenter <dan.carpenter@...cle.com>
CC: "mchehab@...nel.org" <mchehab@...nel.org>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
dl-linux-imx <linux-imx@....com>,
Aisheng Dong <aisheng.dong@....com>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [EXT] Re: [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m
support
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
> Sent: Wednesday, March 9, 2022 7:34 PM
> To: Ming Qian <ming.qian@....com>
> Cc: mchehab@...nel.org; shawnguo@...nel.org; robh+dt@...nel.org;
> s.hauer@...gutronix.de; hverkuil-cisco@...all.nl; kernel@...gutronix.de;
> festevam@...il.com; dl-linux-imx <linux-imx@....com>; Aisheng Dong
> <aisheng.dong@....com>; linux-media@...r.kernel.org;
> linux-kernel@...r.kernel.org; devicetree@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org
> Subject: [EXT] Re: [PATCH v18 06/15] media: amphion: add vpu v4l2 m2m
> support
>
> Caution: EXT Email
>
> This code has a serious case of the u32 pox. There are times where u32 is
> specified in the hardware or network spec. That's when a u32 is appropriate.
> Also for bit masks. Otherwise "int" is normally the correct type. If it's a size
> value then unsigned long, long, or unsigned long long is probably correct.
>
> INT_MAX is just over 2 billion. If you make a number line then most numbers
> are going to be near the zero. You have 10 fingers. You have
> 2 phones. 2 cars. 3 monitors connected to your computer. 200 error
> codes. You're never going to even get close to the 2 billion limit.
>
> For situations where the numbers get very large, then the band on the number
> line between 2 and 4 billion is very narrow. I can name people who have over
> a billion dollars but I cannot name even one who falls exactly between 2 and 4
> billion.
>
> In other words u32 is almost useless for describing anything. If something
> cannot fit in a int then it's not going to fit into a u32 either and you should use
> a u64 instead.
>
> Some people think that unsigned values are more safe than signed values.
> It is true, in certain limited cases that the invisible side effects of unsigned
> math can protect you. But mostly the invisible side effects create surprises
> and bugs. And again if you have to pick an unsigned type pick an u64
> because it is harder to have an integer overflow on a
> 64 bit type vs a 32 bit type.
>
> Avoid u32 types where ever you can, they only cause bugs.
OK, I think you're right, I'll check and fix it according to your comments
Thanks very much for your reivew
>
> > +u32 vpu_helper_copy_from_stream_buffer(struct vpu_buffer
> *stream_buffer,
> > + u32 *rptr, u32 size, void *dst) {
> > + u32 offset;
> > + u32 start;
> > + u32 end;
> > + void *virt;
> > +
> > + if (!stream_buffer || !rptr || !dst)
> > + return -EINVAL;
>
> This function returns negatives.
>
> > +
> > + if (!size)
> > + return 0;
> > +
> > + offset = *rptr;
> > + start = stream_buffer->phys;
> > + end = start + stream_buffer->length;
> > + virt = stream_buffer->virt;
> > +
> > + if (offset < start || offset > end)
> > + return -EINVAL;
> > +
> > + if (offset + size <= end) {
>
> Check for integer overflows?
>
>
> > + memcpy(dst, virt + (offset - start), size);
> > + } else {
> > + memcpy(dst, virt + (offset - start), end - offset);
> > + memcpy(dst + end - offset, virt, size + offset - end);
> > + }
> > +
> > + *rptr = vpu_helper_step_walk(stream_buffer, offset, size);
> > + return size;
>
> This function always returns size on success. Just return 0 on success.
>
> > +}
> > +
> > +u32 vpu_helper_copy_to_stream_buffer(struct vpu_buffer *stream_buffer,
> > + u32 *wptr, u32 size, void *src) {
> > + u32 offset;
> > + u32 start;
> > + u32 end;
> > + void *virt;
> > +
> > + if (!stream_buffer || !wptr || !src)
> > + return -EINVAL;
>
> Signedness bug.
>
> > +
> > + if (!size)
> > + return 0;
> > +
> > + offset = *wptr;
> > + start = stream_buffer->phys;
> > + end = start + stream_buffer->length;
> > + virt = stream_buffer->virt;
> > + if (offset < start || offset > end)
> > + return -EINVAL;
>
> Signedness.
>
> > +
> > + if (offset + size <= end) {
>
> Check for integer overflow?
>
> > + memcpy(virt + (offset - start), src, size);
> > + } else {
> > + memcpy(virt + (offset - start), src, end - offset);
> > + memcpy(virt, src + end - offset, size + offset - end);
> > + }
> > +
> > + *wptr = vpu_helper_step_walk(stream_buffer, offset, size);
> > +
> > + return size;
>
> Just return zero on success. No need to return a known parameter.
>
> > +}
> > +
> > +u32 vpu_helper_memset_stream_buffer(struct vpu_buffer *stream_buffer,
> > + u32 *wptr, u8 val, u32 size) {
> > + u32 offset;
> > + u32 start;
> > + u32 end;
> > + void *virt;
> > +
> > + if (!stream_buffer || !wptr)
> > + return -EINVAL;
>
> Signedness.
>
> > +
> > + if (!size)
> > + return 0;
> > +
> > + offset = *wptr;
> > + start = stream_buffer->phys;
> > + end = start + stream_buffer->length;
> > + virt = stream_buffer->virt;
> > + if (offset < start || offset > end)
> > + return -EINVAL;
> > +
> > + if (offset + size <= end) {
>
> Check for overflow?
>
> > + memset(virt + (offset - start), val, size);
> > + } else {
> > + memset(virt + (offset - start), val, end - offset);
> > + memset(virt, val, size + offset - end);
> > + }
> > +
> > + offset += size;
> > + if (offset >= end)
> > + offset -= stream_buffer->length;
> > +
> > + *wptr = offset;
> > +
> > + return size;
> > +}
>
> regards,
> dan carpenter
Powered by blists - more mailing lists