linux-kernel - Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 26 May 2022 16:20:38 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Eugenio Perez Martin <eperezma@...hat.com>
Cc:     Stefano Garzarella <sgarzare@...hat.com>,
        "Dawar, Gautam" <gautam.dawar@....com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        Jason Wang <jasowang@...hat.com>,
        Zhu Lingshan <lingshan.zhu@...el.com>,
        "martinh@...inx.com" <martinh@...inx.com>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        Eli Cohen <elic@...dia.com>, Parav Pandit <parav@...dia.com>,
        Wu Zongyong <wuzongyong@...ux.alibaba.com>,
        "dinang@...inx.com" <dinang@...inx.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Xie Yongji <xieyongji@...edance.com>,
        "lulu@...hat.com" <lulu@...hat.com>,
        "martinpo@...inx.com" <martinpo@...inx.com>,
        "pabloc@...inx.com" <pabloc@...inx.com>,
        Longpeng <longpeng2@...wei.com>,
        "Piotr.Uminski@...el.com" <Piotr.Uminski@...el.com>,
        "Kamde, Tanuj" <tanuj.kamde@....com>,
        Si-Wei Liu <si-wei.liu@...cle.com>,
        "habetsm.xilinx@...il.com" <habetsm.xilinx@...il.com>,
        "lvivier@...hat.com" <lvivier@...hat.com>,
        Zhang Min <zhang.min9@....com.cn>,
        "hanand@...inx.com" <hanand@...inx.com>
Subject: Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote:
> > >> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
> > >> +       struct vdpa_device *vdpa = v->vdpa;
> > >> +       const struct vdpa_config_ops *ops = vdpa->config;
> > >> +
> > >> +       return ops->stop;
> > >> [GD>>] Would it be better to explicitly return a bool to match the return type?
> > >
> > >I'm not sure about the kernel code style regarding that casting. Maybe
> > >it's better to return !!ops->stop here. The macros likely and unlikely
> > >do that.
> >
> > IIUC `ops->stop` is a function pointer, so what about
> >
> >      return ops->stop != NULL;
> >
> 
> I'm ok with any method proposed. Both three ways can be found in the
> kernel so I think they are all valid (although the double negation is
> from bool to integer in (0,1) set actually).
> 
> Maybe Jason or Michael (as maintainers) can state the preferred method here.

Always just do whatever the person who responded feels like because
they're likely the person who cares the most.  ;)

I don't think there are any static analysis tools which will complain
about this.  Smatch will complain if you return a negative literal.
It feels like returning any literal that isn't 1 or 0 should trigger a
warning...  I've written that and will check it out tonight.

Really anything negative should trigger a warning.  See new Smatch stuff
below.

regards,
dan carpenter

================ TEST CASE =========================

int x;
_Bool one(int *p)
{
        if (p)
                x = -2;
        return x;
}
_Bool two(int *p)
{
        return -4;  // returning 2 triggers a warning now
}

=============== OUTPUT =============================

test.c:10 one() warn: potential negative cast to bool 'x'
test.c:14 two() warn: signedness bug returning '(-4)'
test.c:14 two() warn: '(-4)' is not bool

=============== CODE ===============================

#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"

static int my_id;

static void match_literals(struct expression *ret_value)
{
	struct symbol *type;
	sval_t sval;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	if (!get_implied_value(ret_value, &sval))
		return;

	if (sval.value == 0 || sval.value == 1)
		return;

	sm_warning("'%s' is not bool", sval_to_str(sval));
}

static void match_any_negative(struct expression *ret_value)
{
	struct symbol *type;
	struct sm_state *extra, *sm;
	sval_t sval;
	char *name;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	extra = get_extra_sm_state(ret_value);
	if (!extra)
		return;
	FOR_EACH_PTR(extra->possible, sm) {
		if (estate_get_single_value(sm->state, &sval) &&
		    sval_is_negative(sval)) {
			name = expr_to_str(ret_value);
			sm_warning("potential negative cast to bool '%s'", name);
			free_string(name);
			return;
		}
	} END_FOR_EACH_PTR(sm);
}

void check_bool_return(int id)
{
	my_id = id;

	add_hook(&match_literals, RETURN_HOOK);
	add_hook(&match_any_negative, RETURN_HOOK);
}