[Openipmi-developer] [PATCH] ipmi: fix use after free in _ipmi_destroy_user()
The intf_free() function frees the "intf" pointer so we cannot dereference it again on the next line. Fixes: cbb79863fc31 ("ipmi: Don't allow device module unload when in use") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ipmi_msghandler.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index f6b8ca6df9b5..186f1fee7534 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -1330,6 +1330,7 @@ static void _ipmi_destroy_user(struct ipmi_user *user) unsigned longflags; struct cmd_rcvr *rcvr; struct cmd_rcvr *rcvrs = NULL; + struct module*owner; if (!acquire_ipmi_user(user, )) { /* @@ -1392,8 +1393,9 @@ static void _ipmi_destroy_user(struct ipmi_user *user) kfree(rcvr); } + owner = intf->owner; kref_put(>refcount, intf_free); - module_put(intf->owner); + module_put(owner); } int ipmi_destroy_user(struct ipmi_user *user) -- 2.35.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [linux-next:master] BUILD REGRESSION 088b9c375534d905a4d337c78db3b3bfbb52c4a0
On Thu, Jul 07, 2022 at 07:02:58AM -0700, Guenter Roeck wrote: > and the NULL > dereferences in the binder driver are at the very least suspicious. The NULL dereferences in binder are just nonsense Sparse annotations. They don't affect runtime. drivers/android/binder.c:1481:19-23: ERROR: from is NULL but dereferenced. drivers/android/binder.c:2920:29-33: ERROR: target_thread is NULL but dereferenced. drivers/android/binder.c:353:25-35: ERROR: node -> proc is NULL but dereferenced. drivers/android/binder.c:4888:16-20: ERROR: t is NULL but dereferenced. regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v6 3/4] ipmi: ssif_bmc: Return -EFAULT if copy_from_user() fails
On Thu, Mar 17, 2022 at 02:45:19PM +0700, Quan Nguyen wrote: > Added Dan as I have missed Dan's email address in the first place. > My apologize, > - Quan > > On 11/03/2022 13:58, Wolfram Sang wrote: > > On Thu, Mar 10, 2022 at 06:41:18PM +0700, Quan Nguyen wrote: > > > From: Dan Carpenter > > > > > > The copy_from_user() function returns the number of bytes remaining to > > > be copied but we should return -EFAULT here. > > > > > > Fixes: 501c25b59508 ("ipmi: ssif_bmc: Add SSIF BMC driver") > > > Signed-off-by: Dan Carpenter > > > Signed-off-by: Corey Minyard > > > Signed-off-by: Quan Nguyen > > > > It is nice that you want to keep this patch seperate to give Dan > > credits, but I still think it should be merged into patch 1, so the > > initial driver is as flawless as it can be. You could give Dan still > > credits by mentioning him in the commit message IMO. Dan, would you be > > fine with this? Yes, that's no problem. Thanks! regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
On Wed, Sep 08, 2021 at 08:50:14AM +0200, Marion et Christophe JAILLET wrote: > > > > > Message du 08/09/21 08:28 > > De : "Dan Carpenter" > > A : "Christophe JAILLET" > > Copie à : miny...@acm.org, zwe...@equinix.com, and...@aj.id.au, > > openipmi-developer@lists.sourceforge.net, linux-ker...@vger.kernel.org, > > kernel-janit...@vger.kernel.org > > Objet : Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling > > path of 'kcs_bmc_serio_add_device()' > > > > On Tue, Sep 07, 2021 at 11:06:32PM +0200, Christophe JAILLET wrote: > > > In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()' > > > succeeds, 'port' would be leaking. > > > > > > Test each allocation separately to avoid the leak. > > > > > > Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor") > > > Signed-off-by: Christophe JAILLET > > > --- > > > drivers/char/ipmi/kcs_bmc_serio.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c > > > b/drivers/char/ipmi/kcs_bmc_serio.c > > > index 7948cabde50b..7e2067628a6c 100644 > > > --- a/drivers/char/ipmi/kcs_bmc_serio.c > > > +++ b/drivers/char/ipmi/kcs_bmc_serio.c > > > @@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct > > > kcs_bmc_device *kcs_bmc) > > > struct serio *port; > > > > > > priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > > > > > /* Use kzalloc() as the allocation is cleaned up with kfree() via > > > serio_unregister_port() */ > > > > The serio_unregister_port() calls serio_destroy_port() which calls > > put_device(>dev). But I wasn't able to track it further than > > that to the actual kfree(). > > Hi Dan, > > Checking this release path was not the goal of this patch. Yeah. I was just curious. > It was only about the VERR unlikely memory leak. > > However my understanding is: > kcs_bmc_serio_add_device > --> serio_register_port > --> __serio_register_port > --> serio_init_port > --> serio->dev.release = serio_release_port > > And in serio_release_port: > struct serio *serio = to_serio_port(dev); > kfree(serio); > > For me, this 'serio' looks to the one allocated by 'kcs_bmc_serio_add_device'. > I think that the comment is correct. Thanks. This really helps me actually. I could just make a list of the functions which take a container_of(dev) get a struct serio and then free it. Then if there is only one function that matches that, I could assume it's what put_device() will call. regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: ssif_bmc: Uninitialized return in ssif_bmc_write()
I accidentally introduced a bug in my previous patch. The "ret" variable needs to be initialized to prevent returning uninitialized data. Fixes: f9714eb04364 ("ipmi: ssif_bmc: Return -EFAULT if copy_from_user() fails") Signed-off-by: Dan Carpenter --- Sorry! drivers/char/ipmi/ssif_bmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c index ce8cd8364a3f..acdb1d9cb5c0 100644 --- a/drivers/char/ipmi/ssif_bmc.c +++ b/drivers/char/ipmi/ssif_bmc.c @@ -80,7 +80,7 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file); struct ssif_msg msg; unsigned long flags; - ssize_t ret; + ssize_t ret = 0; if (count > sizeof(struct ssif_msg)) return -EINVAL; -- 2.20.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: ssif_bmc: Return -EFAULT if copy_from_user() fails
The copy_from_user() function returns the number of bytes remaining to be copied but we should return -EFAULT here. Fixes: 007888f365c9 ("ipmi: ssif_bmc: Add SSIF BMC driver") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ssif_bmc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c index b15c05622e72..ce8cd8364a3f 100644 --- a/drivers/char/ipmi/ssif_bmc.c +++ b/drivers/char/ipmi/ssif_bmc.c @@ -85,9 +85,8 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t if (count > sizeof(struct ssif_msg)) return -EINVAL; - ret = copy_from_user(, buf, count); - if (ret) - return ret; + if (copy_from_user(, buf, count)) + return -EFAULT; if (!msg.len || count < ssif_msg_len()) return -EINVAL; -- 2.20.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v3 00/11] Introduce Simple atomic counters
On Fri, Oct 16, 2020 at 03:51:25PM -0700, Kees Cook wrote: > On Fri, Oct 16, 2020 at 12:53:13PM +0200, Peter Zijlstra wrote: > > That's like saying: "I'm too lazy to track what I've looked at already". > > You're basically proposing to graffiti "Kees was here -- 16/10/2020" all > > over the kernel. Just so you can see where you still need to go. > > > > It says the code was (assuming your audit was correct) good at that > > date, but has no guarantees for any moment after that. > > That kind of bit-rot marking is exactly what I would like to avoid: just > putting a comment in is pointless. Making the expectations of the usage > become _enforced_ is the goal. And having it enforced by the _compiler_ > is key. Just adding a meaningless attribute that a static checker > will notice some time and hope people fix them doesn't scale either > (just look at how many sparse warnings there are). Most Sparse warnings are false positives. People do actually fix the ones which matter. I think this patchset could be useful. I'm working on a refcounting check for Smatch. I want to warn about when we forget to drop a reference on an error path. Right now I just assume that anything with "error", "drop" or "->stats->" in the name is just a counter. regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 00/18] use semicolons rather than commas to separate statements
On Tue, Sep 29, 2020 at 02:20:00PM +0200, Ard Biesheuvel wrote: > On Sun, 27 Sep 2020 at 21:56, Julia Lawall wrote: > > > > These patches replace commas by semicolons. > > > Why? > In the best case, these commas are just uninitentional mess, like typing an extra space character or something. I've looked at them before and one case I see where they are introduced is when people convert a struct initializer to code. - struct foo { - .a = 1, - .b = 2, ... + foo.a = 1, + foo.b = 2, The times where commas are used deliberately to replace curly braces are just evil. Either way the code is cleaner with semi-colons. regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: msghandler: Fix a signedness bug
The type for the completion codes should be unsigned char instead of char. If it is declared as a normal char then the conditions in __get_device_id() are impossible because the IPMI_DEVICE_IN_FW_UPDATE_ERR error codes are higher than 127. drivers/char/ipmi/ipmi_msghandler.c:2449 __get_device_id() warn: impossible condition '(bmc->cc == 209) => ((-128)-127 == 209)' Fixes: f8910ffa81b0 ("ipmi:msghandler: retry to get device id on an error") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ipmi_msghandler.c | 2 +- drivers/char/ipmi/ipmi_si_intf.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 555c3b1e4926..8774a3b8ff95 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -319,7 +319,7 @@ struct bmc_device { intdyn_guid_set; struct krefusecount; struct work_struct remove_work; - char cc; /* completion code */ + unsigned char cc; /* completion code */ }; #define to_bmc_device(x) container_of((x), struct bmc_device, pdev.dev) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 164f85007080..0b3dbc7e39fd 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1344,7 +1344,7 @@ static int try_get_dev_id(struct smi_info *smi_info) resp + 2, resp_len - 2, _info->device_id); if (rv) { /* record completion code */ - char cc = *(resp + 2); + unsigned char cc = *(resp + 2); if ((cc == IPMI_DEVICE_IN_FW_UPDATE_ERR || cc == IPMI_DEVICE_IN_INIT_ERR -- 2.28.0 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] block: convert tasklets to use new tasklet_setup() API
On Wed, Aug 26, 2020 at 07:21:35AM +0530, Allen Pais wrote: > On Thu, Aug 20, 2020 at 3:09 AM James Bottomley > wrote: > > > > On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > > > [...] > > > > > > Since both threads seem to have petered out, let me suggest in > > > > > > kernel.h: > > > > > > > > > > > > #define cast_out(ptr, container, member) \ > > > > > > container_of(ptr, typeof(*container), member) > > > > > > > > > > > > It does what you want, the argument order is the same as > > > > > > container_of with the only difference being you name the > > > > > > containing structure instead of having to specify its type. > > > > > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > > > cast_out, it's not very descriptive. And it has connotations of > > > > > getting rid of something, which isn't really true. > > > > > > > > Um, I thought it was exactly descriptive: you're casting to the > > > > outer container. I thought about following the C++ dynamic casting > > > > style, so out_cast(), but that seemed a bit pejorative. What about > > > > outer_cast()? > > > > > > > > > FWIW, I like the from_ part of the original naming, as it has > > > > > some clues as to what is being done here. Why not just > > > > > from_container()? That should immediately tell people what it > > > > > does without having to look up the implementation, even before > > > > > this becomes a part of the accepted coding norm. > > > > > > > > I'm not opposed to container_from() but it seems a little less > > > > descriptive than outer_cast() but I don't really care. I always > > > > have to look up container_of() when I'm using it so this would just > > > > be another macro of that type ... > > > > > > > > > > So far we have a few which have been suggested as replacement > > > for from_tasklet() > > > > > > - out_cast() or outer_cast() > > > - from_member(). > > > - container_from() or from_container() > > > > > > from_container() sounds fine, would trimming it a bit work? like > > > from_cont(). > > > > I'm fine with container_from(). It's the same form as container_of() > > and I think we need urgent agreement to not stall everything else so > > the most innocuous name is likely to get the widest acceptance. > > Kees, > > Will you be sending the newly proposed API to Linus? I have V2 > which uses container_from() > ready to be sent out. I liked that James swapped the first two arguments so that it matches container_of(). Plus it's nice that when you have: struct whatever *foo = container_from(ptr, foo, member); Then it means that "ptr == >member". regards, dan carpenter ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: kcs: Fix aspeed_kcs_probe_of_v1()
This needs to return the newly allocated struct but instead it returns zero which leads to an immediate Oops in the caller. Fixes: 09f5f680707e ("ipmi: kcs: aspeed: Implement v2 bindings") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/kcs_bmc_aspeed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index 9422d55a0476..a140203c079b 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -271,7 +271,7 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev) kcs->ioreg = ast_kcs_bmc_ioregs[channel - 1]; aspeed_kcs_set_address(kcs, slave); - return 0; + return kcs; } static int aspeed_kcs_calculate_channel(const struct kcs_ioreg *regs) -- 2.25.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi_ssif: Fix locking on probe error path
If the allocations failed then we returned with the lock held. This patch moves the allocations infront of the locking. Fixes: 714acbc6cc39 ("ipmi_ssif: avoid registering duplicate ssif interface") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ipmi_ssif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 205726926bd3..9cf2efd33f19 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1683,7 +1683,6 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) u8slave_addr = 0; struct ssif_addr_info *addr_info = NULL; - mutex_lock(_infos_mutex); resp = kmalloc(IPMI_MAX_MSG_LENGTH, GFP_KERNEL); if (!resp) return -ENOMEM; @@ -1694,6 +1693,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) return -ENOMEM; } + mutex_lock(_infos_mutex); + if (!check_acpi(ssif_info, >dev)) { addr_info = ssif_info_find(client->addr, client->adapter->name, true); -- 2.20.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi_si: remove an unused variable in try_smi_init()
The "init_name" variable isn't used any more after commit 90b2d4f15ff7 ("ipmi_si: Remove hacks for adding a dummy platform devices"). Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ipmi_si_intf.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index b1732882b97e..f124a2d2bb9f 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1931,7 +1931,6 @@ static int try_smi_init(struct smi_info *new_smi) { int rv = 0; int i; - char *init_name = NULL; pr_info("Trying %s-specified %s state machine at %s address 0x%lx, slave address 0x%x, irq %d\n", ipmi_addr_src_to_str(new_smi->io.addr_source), @@ -2073,7 +2072,6 @@ static int try_smi_init(struct smi_info *new_smi) new_smi->io.io_cleanup = NULL; } - kfree(init_name); return rv; } -- 2.17.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi_si: Potential array underflow in hotmod_handler()
The "ival" variable needs to signed so that we don't read before the start of the str[] array. This would only happen the user passed in a module parameter that was just comprised of space characters. Fixes: e80444ae4fc3 ("ipmi_si: Switch hotmod to use a platform device") Signed-off-by: Dan Carpenter --- drivers/char/ipmi/ipmi_si_hotmod.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_si_hotmod.c b/drivers/char/ipmi/ipmi_si_hotmod.c index 1433055a9705..03140f6cdf6f 100644 --- a/drivers/char/ipmi/ipmi_si_hotmod.c +++ b/drivers/char/ipmi/ipmi_si_hotmod.c @@ -187,7 +187,8 @@ static int hotmod_handler(const char *val, const struct kernel_param *kp) char *str = kstrdup(val, GFP_KERNEL), *curr, *next; int rv; struct ipmi_plat_data h; - unsigned int len, ival; + unsigned int len; + int ival; if (!str) return -ENOMEM; -- 2.17.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: msghandler: ensure error value in rv is being returned on error
On Tue, Aug 28, 2018 at 03:36:42PM +0100, Colin King wrote: > From: Colin Ian King > > Currently ipmi_set_my_LUN returns 0 instead of -EINVAL if the channel > is out of range. Fix this by returning the error return rv instead of 0. > > Detected by Clang ("variable 'rv' set but not used") > > Fixes: 048f7c3e352e ("ipmi: Properly release srcu locks on error conditions") > Signed-off-by: Colin Ian King Somebody just sent that some hours ago. regards, dan carpenter -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH v2] ipmi: Fix error code in try_smi_init()
If platform_device_alloc() fails, then we should return -ENOMEM instead of returning success. I've also removed the "rv" initialization so that GCC can maybe detect if we forget to set it in the future. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> --- v2: Don't initialize "rv" and fix a typo in the commit message diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index f2a294f78892..c09b279cd55e 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2026,7 +2026,7 @@ int ipmi_si_add_smi(struct si_sm_io *io) */ static int try_smi_init(struct smi_info *new_smi) { - int rv = 0; + int rv; int i; char *init_name = NULL; @@ -2071,6 +2071,7 @@ static int try_smi_init(struct smi_info *new_smi) new_smi->intf_num); if (!new_smi->pdev) { pr_err(PFX "Unable to allocate platform device\n"); + rv = -ENOMEM; goto out_err; } new_smi->io.dev = _smi->pdev->dev; -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: missing error code in try_smi_init()
On Tue, Mar 06, 2018 at 11:26:19AM +0100, Julia Lawall wrote: > > > On Tue, 6 Mar 2018, Dan Carpenter wrote: > > > If platform_device_alloc() then we should return -ENOMEM instead of > > returning success. > > It looks like the word "fails" got lost here. > Thanks. Let me resend. regards, dan carpenter -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: missing error code in try_smi_init()
If platform_device_alloc() then we should return -ENOMEM instead of returning success. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index f2a294f78892..ff870aa91cfe 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2071,6 +2071,7 @@ static int try_smi_init(struct smi_info *new_smi) new_smi->intf_num); if (!new_smi->pdev) { pr_err(PFX "Unable to allocate platform device\n"); + rv = -ENOMEM; goto out_err; } new_smi->io.dev = _smi->pdev->dev; -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [bug report] ipmi: Add SMBus interface driver (SSIF)
; 813 } 814 break; 815 816 case SSIF_GETTING_MESSAGES: 817 if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) { 818 /* Error getting event, probably done. */ 819 msg->done(msg); 820 821 /* Take off the msg flag. */ 822 ssif_info->msg_flags &= ~RECEIVE_MSG_AVAIL; 823 handle_flags(ssif_info, flags); 824 } else if (msg->rsp[0] != (IPMI_NETFN_APP_REQUEST | 1) << 2 825 || msg->rsp[1] != IPMI_GET_MSG_CMD) { 826 pr_warn(PFX "Invalid response clearing flags: %x %x\n", 827 msg->rsp[0], msg->rsp[1]); 828 msg->done(msg); 829 830 /* Take off the msg flag. */ 831 ssif_info->msg_flags &= ~RECEIVE_MSG_AVAIL; 832 handle_flags(ssif_info, flags); 833 } else { 834 ssif_inc_stat(ssif_info, incoming_messages); 835 handle_flags(ssif_info, flags); 836 deliver_recv_msg(ssif_info, msg); 837 } 838 break; 839 } 840 841 flags = ipmi_ssif_lock_cond(ssif_info, ); ^^ Deadlock. 842 if (SSIF_IDLE(ssif_info) && !ssif_info->stopping) { 843 if (ssif_info->req_events) regards, dan carpenter -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi_ssif: unlock on allocation failure
We should unlock and re-enable IRQs if this allocation fails. Fixes: 259307074bfc ("ipmi: Add SMBus interface driver (SSIF) ") Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 0b22a9be5029..6dd6476ea5d3 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -408,6 +408,7 @@ static void start_event_fetch(struct ssif_info *ssif_info, unsigned long *flags) msg = ipmi_alloc_smi_msg(); if (!msg) { ssif_info->ssif_state = SSIF_NORMAL; + ipmi_ssif_unlock_cond(ssif_info, flags); return; } @@ -430,6 +431,7 @@ static void start_recv_msg_fetch(struct ssif_info *ssif_info, msg = ipmi_alloc_smi_msg(); if (!msg) { ssif_info->ssif_state = SSIF_NORMAL; + ipmi_ssif_unlock_cond(ssif_info, flags); return; } -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [bug report] ipmi: shift wrap warning in STORE_SEQ_IN_MSGID()
Hi Corey, The patch 1da177e4c3f4: "Linux-2.6.12-rc2" from Apr 16, 2005, leads to the following static checker warning: drivers/char/ipmi/ipmi_msghandler.c:1760 i_ipmi_request() warn: potential shift truncation. '0xff << 26' drivers/char/ipmi/ipmi_msghandler.c 157 /* 158 * Store the information in a msgid (long) to allow us to find a 159 * sequence table entry from the msgid. 160 */ 161 #define STORE_SEQ_IN_MSGID(seq, seqid) (((seq&0xff)<<26) | (seqid&0x3ff)) The STORE_SEQ_IN_MSGID() and GET_SEQ_FROM_MSGID() don't match. It seems clear enough that it should be (((seq & 0x3f) << 26) and that would silence the static checker warning. But I think also that in GET_SEQ_FROM_MSGID() and NEXT_SEQID() we should be using 0x3ff (with 6 f's instead of 5). 162 163 #define GET_SEQ_FROM_MSGID(msgid, seq, seqid) \ 164 do { \ 165 seq = ((msgid >> 26) & 0x3f); \ 166 seqid = (msgid & 0x3f); \ 167 } while (0) 168 169 #define NEXT_SEQID(seqid) (((seqid) + 1) & 0x3f) 170 regards, dan carpenter -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [patch] ipmi_ssif: silence an uninitialized variable warning
My static checker complains that "ret" could be zero here. (Presumably that means the hardware is badly busted). But the result would be that the caller assumes *resp_len is initialized when it's not and it leads to a warning. Let's just silence the warning. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 8b3be8b..512c5b6 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1232,6 +1232,8 @@ static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, break; } + if (ret == 0) + ret = -EINVAL; if (ret > 0) { /* Validate that the response is correct. */ if (ret < 3 || -- Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [patch v2] ipmi: info leak in compat_ipmi_ioctl()
On x86_64 there is a 4 byte hole between -recv_type and -addr. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com --- v2: fixed the changelog a little. Also added LKML because the openipmi is a moderated list (and the moderator thought my email was spam). diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c index 9eb360f..8e306ac 100644 --- a/drivers/char/ipmi/ipmi_devintf.c +++ b/drivers/char/ipmi/ipmi_devintf.c @@ -810,6 +810,7 @@ static long compat_ipmi_ioctl(struct file *filep, unsigned int cmd, struct ipmi_recv __user *precv64; struct ipmi_recv recv64; + memset(recv64, 0, sizeof(recv64)); if (get_compat_ipmi_recv(recv64, compat_ptr(arg))) return -EFAULT; -- Get 100% visibility into Java/.NET code with AppDynamics Lite It's a free troubleshooting tool designed for production Get down to code-level detail for bottlenecks, with 2% overhead. Download for free and get started troubleshooting in minutes. http://p.sf.net/sfu/appdyn_d2d_ap2 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer