| Message ID | 1420556647-17270-1-git-send-email-yang.jie@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 06, 2015 at 11:04:07PM +0800, Jie Yang wrote: > We should not send IPC stream commands to FW when the stream is > NULL, dereference the NULL pointer may also occur without precheck. > Here add NULL pointer checks for these stream APIs. > @@ -1420,6 +1423,9 @@ int sst_hsw_stream_commit(struct sst_hsw *hsw, struct sst_hsw_stream *stream) > u32 header; > int ret; > > + if (stream->commited) > + return 0; > + > trace_ipc_request("stream alloc", stream->host_id); > > header = IPC_GLB_TYPE(IPC_GLB_ALLOCATE_STREAM); This is a bit worrying, it means that we will silently ignore any attempts to pass in a NULL stream. Sometimes that's an OK thing to do (like in deallocation paths) but this seems to more generally silently ignore errors which means we're less likely to spot other coding issues. Race conditions worry me especially - trying to use something before we've quite allocated it for example. I'd be much happier if this warned in cases where it wasn't totally obvious that attempting to use a NULL pointer is sensible.
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Wednesday, January 07, 2015 1:22 AM > To: Jie, Yang > Cc: alsa-devel@alsa-project.org; Girdwood, Liam R > Subject: Re: [PATCH] ASoC: Intel: Add NULL checks for the stream pointer > > On Tue, Jan 06, 2015 at 11:04:07PM +0800, Jie Yang wrote: > > > We should not send IPC stream commands to FW when the stream is NULL, > > dereference the NULL pointer may also occur without precheck. > > Here add NULL pointer checks for these stream APIs. > > > @@ -1420,6 +1423,9 @@ int sst_hsw_stream_commit(struct sst_hsw *hsw, > struct sst_hsw_stream *stream) > > u32 header; > > int ret; > > > > + if (stream->commited) > > + return 0; > > + > > trace_ipc_request("stream alloc", stream->host_id); > > > > header = IPC_GLB_TYPE(IPC_GLB_ALLOCATE_STREAM); > > This is a bit worrying, it means that we will silently ignore any attempts to pass > in a NULL stream. Sometimes that's an OK thing to do (like in deallocation > paths) but this seems to more generally silently ignore errors which means > we're less likely to spot other coding issues. > Race conditions worry me especially - trying to use something before we've > quite allocated it for example. > > I'd be much happier if this warned in cases where it wasn't totally obvious that > attempting to use a NULL pointer is sensible. [Keyon] thanks for review, Mark. So you meant we should add some warning log here, right? ~Keyon
On Wed, Jan 07, 2015 at 12:53:24AM +0000, Jie, Yang wrote: > > I'd be much happier if this warned in cases where it wasn't totally obvious that > > attempting to use a NULL pointer is sensible. > [Keyon] thanks for review, Mark. So you meant we should add some warning log here, right? Yes, I'd be a lot happier with that - if there's some cases which happen a lot and it makes sense that they happen a lot then the changelog and/or the code should make it clear that those cases are OK but as a default it seems better to at least say we ignored things.
diff --git a/sound/soc/intel/sst-haswell-ipc.c b/sound/soc/intel/sst-haswell-ipc.c index 57d6e89..3fd9838 100644 --- a/sound/soc/intel/sst-haswell-ipc.c +++ b/sound/soc/intel/sst-haswell-ipc.c @@ -1233,6 +1233,9 @@ int sst_hsw_stream_free(struct sst_hsw *hsw, struct sst_hsw_stream *stream) struct sst_dsp *sst = hsw->dsp; unsigned long flags; + if (!stream) + return 0; + /* dont free DSP streams that are not commited */ if (!stream->commited) goto out; @@ -1420,6 +1423,9 @@ int sst_hsw_stream_commit(struct sst_hsw *hsw, struct sst_hsw_stream *stream) u32 header; int ret; + if (stream->commited) + return 0; + trace_ipc_request("stream alloc", stream->host_id); header = IPC_GLB_TYPE(IPC_GLB_ALLOCATE_STREAM); @@ -1548,6 +1554,9 @@ int sst_hsw_stream_pause(struct sst_hsw *hsw, struct sst_hsw_stream *stream, { int ret; + if (!stream) + return 0; + trace_ipc_request("stream pause", stream->reply.stream_hw_id); ret = sst_hsw_stream_operations(hsw, IPC_STR_PAUSE, @@ -1564,6 +1573,9 @@ int sst_hsw_stream_resume(struct sst_hsw *hsw, struct sst_hsw_stream *stream, { int ret; + if (!stream) + return 0; + trace_ipc_request("stream resume", stream->reply.stream_hw_id); ret = sst_hsw_stream_operations(hsw, IPC_STR_RESUME, @@ -1579,6 +1591,9 @@ int sst_hsw_stream_reset(struct sst_hsw *hsw, struct sst_hsw_stream *stream) { int ret, tries = 10; + if (!stream) + return 0; + /* dont reset streams that are not commited */ if (!stream->commited) return 0;
We should not send IPC stream commands to FW when the stream is NULL, dereference the NULL pointer may also occur without precheck. Here add NULL pointer checks for these stream APIs. Signed-off-by: Jie Yang <yang.jie@intel.com> --- sound/soc/intel/sst-haswell-ipc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)