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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 3 May 2020 09:06:34 +0200 From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org> To: "Daniel W. S. Almeida" <dwlsalmeida@...il.com> Cc: sean@...s.org, kstewart@...uxfoundation.org, allison@...utok.net, tglx@...utronix.de, linux-media@...r.kernel.org, skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linuxfoundation.org, linux-kernel@...r.kernel.org Subject: Re: [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Em Sat, 2 May 2020 08:40:38 +0200 Mauro Carvalho Chehab <mchehab+huawei@...nel.org> escreveu: > Em Sat, 2 May 2020 00:22:11 -0300 > "Daniel W. S. Almeida" <dwlsalmeida@...il.com> escreveu: > > > +u32 vidtv_memcpy(void *to, > > + const void *from, > > + size_t len, > > + u32 offset, > > + u32 buf_sz) > > +{ > > + if (buf_sz && offset + len > buf_sz) { > > + pr_err("%s: overflow detected, skipping. Try increasing the buffer size", > > + __func__); > > + return 0; > > shouldn't it return an error? > > > + } > > + > > + memcpy(to, from, len); > > + return len; > > +} When trying to use your memset wrapper, I noticed a few issues there. The first one is that you should not use __func__ directly at pr_* macros. Instead, just ensure that you have a pr_fmt() macro that ill be adding the driver's name and the function, e. g.: #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ Besides that, the parameter order sounded weird: > > +u32 vidtv_memcpy(void *to, > > + const void *from, > > + size_t len, > > + u32 offset, > > + u32 buf_sz) The "to", "offset" and "buf_sz" arguments refer to the "to" buffer, while "from" and "len" refers to what will be copied from the "from" into the "to" buffer. Please re-order it, placing first the "to" args, then "from" arg, and finally the argument that applies to both, e. g.: size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size, const void *from, size_t len) (you should notice that I'm using size_t for all args there). The same is also valid for the memset. Finally, the places where this function is used require to pass twice the offset (from patch 08/11): > + nbytes += vidtv_memcpy(args.dest_buf + > + args.dest_offset + > + nbytes, > + &ts_header, > + sizeof(ts_header), > + args.dest_offset + nbytes, > + args.dest_buf_sz); That doesn't make much sense. I mean, if the arguments are "buf + offset", one would expect that the "buf" would be the head of a buffer, and the function would be adding the offset internally. So, the best would be to re-define it like: /** * vidtv_memcpy() - wrapper routine to be used by MPEG-TS * generator, in order to avoid going past the * output buffer. * @to: Starting element to where a MPEG-TS packet will * be copied. * @to_offset: Starting position of the @to buffer to be filled. * @to_size: Size of the @to buffer. * @from: Starting element of the buffer to be copied. * @ten: Number of elements to be copy from @from buffer * into @to+ @to_offset buffer. * * Note: * Real digital TV demod drivers should not have memcpy * wrappers. We use it here just because emulating a MPEG-TS * generation at kernelspace require some extra care. * * Return: * Returns the number of bytes */ size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size, const void *from, size_t len) { if unlikely(to_offset + len > to_size) { pr_err_ratelimited("overflow detected, skipping. Try increasing the buffer size\n"); return 0; } memcpy(to + to_offset, from, len); return len; } Thanks, Mauro
Powered by blists - more mailing lists